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

[Sean Mullan]

Looks good. My only question is whether the apiNote should be an 
implNote instead since it refers to what the JDK Implementation does. 
But either way seems ok.

--Sean

On 11/26/18 1:14 PM, Xue-Lei Fan wrote:
> I made the update accordingly:
> 
>    http://cr.openjdk.java.net/~xuelei/8210985/webrev.04/
> 
> Thanks,
> 
> Xuelei
> 
> On 11/19/2018 7:39 AM, Sean Mullan wrote:
>> 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