fix parameter timeout checks, rename timeOut to retryTimeout

[EdColeman]

Fixes parameter validation of timeout where the instance variable timeOut was used.
Renames instance variable timeOut to retryTimeout to reflect the intended purpose in the javadoc.

This replaces #3166.

There may be follow-on work to rename the setter / getters for timeout as a follow-on and this PR helps distinguish variations in timeout used in the code, As is this a minimal set of changes for quick review.

[ddanielr]

Does it make sense to also update batchTimeOut to match the case of retryTimeout for uniformity?

[dlmarion]

It appears that the use of timeout between Scanner and BatchScanner is inconsistent. Even though they use the same API method the BatchScanner actually times out. This is an issue we should fix. This PR is step 1 I think.

[EdColeman]

Yes - this is step one with the minimum to fix the parameter checks and a little renaming to help with resolving the API conflict.

[ddanielr]

👍 Changing the verbiage to retryTimeout makes its use much clearer!

[dlmarion]

The BatchScanner interface overrides the ScannerBase interface method setTimeout and has a different definition for it. I'm wondering if it would be better to go one step further here and change ScannerBase. For example:

diff --git a/core/src/main/java/org/apache/accumulo/core/client/ScannerBase.java b/core/src/main/java/org/apache/accumulo/core/client/ScannerBase.java
index 5270c45221..a2f9a22c46 100644
--- a/core/src/main/java/org/apache/accumulo/core/client/ScannerBase.java
+++ b/core/src/main/java/org/apache/accumulo/core/client/ScannerBase.java
@@ -208,17 +208,47 @@ public interface ScannerBase extends Iterable<Entry<Key,Value>>, AutoCloseable {
    * @param timeOut the length of the timeout
    * @param timeUnit the units of the timeout
    * @since 1.5.0
+   * @deprecated - replaced by {@link #setRetryTimeout(long, TimeUnit)}
    */
-  void setTimeout(long timeOut, TimeUnit timeUnit);
+  @Deprecated
+  default void setTimeout(long timeOut, TimeUnit timeUnit) {
+    setRetryTimeout(timeOut, timeUnit);
+  }
 
   /**
    * Returns the setting for how long a scanner will automatically retry when a failure occurs.
    *
    * @return the timeout configured for this scanner
    * @since 1.5.0
+   * @deprecated - replaced by {@link #getRetryTimeout(TimeUnit)}
    */
-  long getTimeout(TimeUnit timeUnit);
+  @Deprecated
+  default long getTimeout(TimeUnit timeUnit) {
+    return getRetryTimeout(timeUnit);
+  }
 
+  /**
+   * This setting determines how long a scanner will automatically retry when a failure occurs. By
+   * default, a scanner will retry forever.
+   *
+   * <p>
+   * Setting the timeout to zero (with any time unit) or {@link Long#MAX_VALUE} (with
+   * {@link TimeUnit#MILLISECONDS}) means no timeout.
+   *
+   * @param timeOut the length of the timeout
+   * @param timeUnit the units of the timeout
+   * @since 2.1.1
+   */
+  void setRetryTimeout(long retryTimeout, TimeUnit timeUnit);
+  
+  /**
+   * Returns the setting for how long a scanner will automatically retry when a failure occurs.
+   *
+   * @return the timeout configured for this scanner
+   * @since 2.1.1
+   */
+  long getRetryTimeout(TimeUnit timeUnit);
+  
   /**
    * Closes any underlying connections on the scanner. This may invalidate any iterators derived
    * from the Scanner, causing them to throw exceptions.

You would make the same changes that you have here already, changing the ScannerOptions variable from timeOut to retryTimeout and renaming the ScannerOptions set/getTimeout method. However, TabletServerBatchReader would need it's own setTimeout implementation and timeout variable for its different use case.

[EdColeman]

I intend to tackle this next. The renaming here should help make sure the next changes are correct and easier to review.

[dlmarion]

ok, when you say next, you are adding to this PR?

[EdColeman]

No - my preference is to commit this and then move on to keep the changes isolated - it was tough to track chages when I tried to deprecate setTimeout and use setRetryTimeout with the conflict with batch override

[ctubbsii]

Might want to check to see if the bug affects 1.10 also, so it can be patched, too.

[EdColeman]

This looks like 1.10 may be a better target - but the package names have changed - @ctubbsii do you have a preference for the way the changes are applied? I could try an see if I can use the UI to target 1.10, close this and make the same changes in 1.10 and then merge forward,...?

The base branch was changed.

[EdColeman]

Replaced with #3176 that is based on 1.10.