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

[Xue-Lei Fan]

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