Code Review Request, JDK-8213577, Update the default SSL session cache size to 20480

[Sean Mullan]

On 11/16/18 3:19 PM, Xuelei Fan wrote:
> Thanks for the review, Jmail & Sean.
> 
> New webrev:
>      http://cr.openjdk.java.net/~xuelei/8210985/webrev.03/
> 
> I will update CSR when we come to an agreement.
> 
> On 11/16/2018 11:33 AM, Sean Mullan wrote:
>>   122      * @apiNote Both the session timeout and cache size impact 
>> performance
>>   123      *          of future connections.  It is not recommended to 
>> use too big
>>   124      *          or too small timeout or cache size limit. 
>> Applications should
>>   125      *          carefully weigh the limits and performance for 
>> the specific
>>   126      *          circumstances.
>>
>> I am not really sure if the @apiNote is that useful or appropriate,
> I worry about the default value actually.

Then maybe the default is what you should be discussing in this apiNote. 
Right now I don't think the apiNote adds much. To me, all you are really 
saying is that these are methods that can be used to tune performance, 
which I think should be obvious from their name and description. Maybe 
the apiNote should say something like:

"Note that the JDK Implementation uses default values for both the 
session cache size and timeout. See getSessionCacheSize and 
getSessionTimeout for more information. Applications should consider 
their performance requirements and override the defaults if necessary."

Also I think you should add a similar @implNote for getSessionTimeout 
which describes the default value (86400 seconds or 24 hours), ex:

@implNote The JDK implementation returns the session timeout as set by 
        the {@code setSessionTimeout} method, or if not set, a default 
value of 86400 seconds (24 hours).

>  A new bug may be filed again 
> and argue if the default value is not a proper one.  The default value 
> of session timeout and cache size really depends on the real world 
> circumstances.  I think we'd better make a clarification in the spec, 
> and remind application tune them accordingly.

Ok, but the apiNote above says nothing about the default value.

--Sean

> 
>> but it seems a bit odd to talk about the session timeout in this method. 
> The performance impact is a combination of the session timeout limit and 
> cache size.  I would like application consider them together if need to 
> tune the values, but not individually.
> 
>> If you still want to add this, I would add an @apiNote to each of the 
>> setSessionCacheSize and setSessionTimeout methods and just discuss 
>> each property separately.
>>
> I added the update spec to both setSessionCacheSize and setSessionTimeout.
> 
>> Also, unless you say what "too big" or "too small" is, I would avoid 
>> giving this advice.
>>
> It makes sense to me.
> 
> Thanks,
> Xuelei
> 
>> --Sean
>>
>>
>> On 11/16/18 1:30 PM, Xuelei Fan wrote:
>>> It's time to use the systemProperty tag as it is ready.
>>>
>>> As we are already there, I also update the setSessionCacheSize() for 
>>> more clarification.
>>>
>>> Please review both CSR and webrev:
>>>      https://bugs.openjdk.java.net/browse/JDK-8213577
>>>      http://cr.openjdk.java.net/~xuelei/8210985/webrev.02/
>>>
>>> Thanks,
>>> Xuelei
>>>
>>> On 11/16/2018 8:19 AM, Sean Mullan wrote:
>>>> On 11/15/18 3:37 PM, Xuelei Fan wrote:
>>>>> Hi Sean,
>>>>>
>>>>> Are you OK if we do it later?  I'm waiting for the @systemProperty 
>>>>> tag, proposed within JDK-5076751.  I will file a bug to use the tag 
>>>>> for more cleanup.
>>>>
>>>> JDK-5076751 is completed and pushed to JDK 12, so you can use the 
>>>> new tag now.
>>>>
>>>> I think it would be easier to do it now, it seems pretty simple and 
>>>> that way there is no need to worry about it later.
>>>>
>>>> --Sean
>>>>
>>>>>
>>>>> Thanks,
>>>>> Xuelei
>>>>>
>>>>> On 11/15/2018 11:55 AM, Sean Mullan wrote:
>>>>>> This is a good opportunity to document the 
>>>>>> javax.net.ssl.sessionCacheSize system property in the 
>>>>>> SSLSessionContext API (and use the new @systemProperty tag) in an 
>>>>>> @implNote, for example:
>>>>>>
>>>>>>      /**
>>>>>>       * Returns the size of the cache used for storing
>>>>>>       * <code>SSLSession</code> objects grouped under this
>>>>>>       * <code>SSLSessionContext</code>.
>>>>>>       *
>>>>>>       * @implNote The JDK implementation returns the cache size as 
>>>>>> set by
>>>>>>       * the {@code setSessionCacheSize method}, or if not set, the 
>>>>>> value
>>>>>>       * of the {@systemProperty javax.net.ssl.sessionCacheSize} 
>>>>>> system
>>>>>>       * property. If neither is set, it returns a default value of 
>>>>>> 20480.
>>>>>>       *
>>>>>>       * @return size of the session cache; zero means there is no 
>>>>>> size limit.
>>>>>>       * @see #setSessionCacheSize
>>>>>>       */
>>>>>>      public int getSessionCacheSize();
>>>>>>
>>>>>> On 11/14/18 11:59 AM, Xuelei Fan wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Please review this simple update:
>>>>>>>      http://cr.openjdk.java.net/~xuelei/8210985/webrev.00/
>>>>>>>
>>>>>>> The default value for the maximum number of entries in the SSL 
>>>>>>> session cache (SSLSessionContext.getSessionCacheSize()) is 
>>>>>>> infinite now.  In the request, the default value is updated to 
>>>>>>> 20480 for performance consideration.
>>>>>>>
>>>>>>> For the detailed behavior update, please refer to CSR:
>>>>>>>      https://bugs.openjdk.java.net/browse/JDK-8213577
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Xuelei