Accept long instead of int in SocketSettings.Builder.connectTimeout/readTimeout
#1279
Conversation
…out`/`readTimeout` JAVA-3897
| @@ -93,25 +95,27 @@ public Builder applySettings(final SocketSettings socketSettings) { | |||
| /** | |||
| * Sets the socket connect timeout. | |||
| * | |||
| * @param connectTimeout the connect timeout | |||
| * @param connectTimeout the connect timeout. | |||
| * The timeout converted to milliseconds must not be greater than {@link Integer#MAX_VALUE}. | |||
There was a problem hiding this comment.
We had this requirement for forever (the toIntExact conversion does not, therefore, introduce a new restriction), but did not document it. Now it is documented.
| int result = (int) (connectTimeoutMS ^ (connectTimeoutMS >>> 32)); | ||
| result = 31 * result + (int) (readTimeoutMS ^ (readTimeoutMS >>> 32)); | ||
| result = 31 * result + receiveBufferSize; | ||
| result = 31 * result + sendBufferSize; | ||
| result = 31 * result + proxySettings.hashCode(); | ||
| return result; |
There was a problem hiding this comment.
The bit shifting in the old code became irrelevant, i.e., hashCode had to be updated. I updated it to use Objects.hash.
| private int connectTimeoutMS = 10000; | ||
| private int readTimeoutMS; |
There was a problem hiding this comment.
There is no reason to have the fields internally of the long type, because the millisecond values were not allowed (and will continue being not allowed) to be greater than Integer.MAX_VALUE.
| public Builder connectTimeout(final int connectTimeout, final TimeUnit timeUnit) { | ||
| this.connectTimeoutMS = MILLISECONDS.convert(connectTimeout, timeUnit); | ||
| public Builder connectTimeout(final long connectTimeout, final TimeUnit timeUnit) { | ||
| this.connectTimeoutMS = toIntExact(MILLISECONDS.convert(connectTimeout, timeUnit)); |
There was a problem hiding this comment.
Would this be a new thing to have the driver throw ArithmeticException on an invalid value from the application? Should the exception instead be wrapped in IllegalArgumentException, making it similar to ConnectionString?
| public Builder connectTimeout(final int connectTimeout, final TimeUnit timeUnit) { | ||
| this.connectTimeoutMS = MILLISECONDS.convert(connectTimeout, timeUnit); | ||
| public Builder connectTimeout(final long connectTimeout, final TimeUnit timeUnit) { | ||
| this.connectTimeoutMS = toIntExact(MILLISECONDS.convert(connectTimeout, timeUnit)); |
There was a problem hiding this comment.
Consider adding a test for the new behavior.
… test the setters JAVA-3897
| @@ -21,7 +21,9 @@ import spock.lang.Specification | |||
|
|
|||
| import static java.util.concurrent.TimeUnit.MILLISECONDS | |||
|
|
|||
|
|
|||
| /** | |||
| * New unit tests for {@link SocketSettings} are to be added to {@link SocketSettingsTest}. | |||
There was a problem hiding this comment.
The team discussed and made a decision to create new tests in Java where this is not too much of a hassle, even if there are existing tests in Groovy.
JAVA-3897