Investigate replacing long+TimeUnit with Duration in more places

[DomGarguilo]

Is your feature request related to a problem? Please describe. No problem. It seems like a nice improvement to replace methods that accept a long and a TimeUnit with a single Duration parameter.

Looking through the code I see the following methods that have this pattern of long+TimeUnit: EDIT: crossed out ones that we shouldnt change

• ~~ScannerBase.setTimeout(long timeOut, TimeUnit timeUnit)~~
• ~~ScannerBase.setBatchTimeout(long timeOut, TimeUnit timeUnit)~~
• ~~BatchWriterConfig.setMaxLatency(long maxLatency, TimeUnit timeUnit)~~
• ~~BatchWriterConfig.setTimeout(long timeout, TimeUnit timeUnit)~~
• ~~ConditionalWriterConfig.setTimeout(long timeout, TimeUnit timeUnit)~~
• ~~DelegationTokenConfig.setTokenLifetime(long lifetime, TimeUnit unit)~~
• TabletLogger.suspended(KeyExtent extent, HostAndPort server, long time, TimeUnit timeUnit, int numWalogs)
• ThreadPools.createFixedThreadPool(int numThreads, long timeOut, TimeUnit units, final String name, boolean emitThreadPoolMetrics)
• ThreadPools.createThreadPool
• ~~MiniAccumuloCluster.stopProcessWithTimeout(final Process proc, long timeout, TimeUnit unit)~~
• TestTicker.advance(final long value, final TimeUnit units)

There is a good chance that a lot of these should stay as they are or are not worth looking into but it seems like this could be a good improvement for some. I think each would need to be investigated to see how these values are used and if there are reasons why we might want to avoid Duration.

[DomGarguilo]

This ticket should probably wait until #4360 is either completed or closed.

[keith-turner]

I don't think we should make this change to the public API methods if we would deprecated the existing methods because it would introduce work for those who have written code written against these APIs w/o offering them a strong benefit for that work.

[DomGarguilo]

I don't think we should make this change to the public API methods if we would deprecated the existing methods because it would introduce work for those who have written code written against these APIs w/o offering them a strong benefit for that work.

Good point. I have edited the description to cross those out. There are just a few remaining now. I'll look around to see if there would be a good reason to not change these.

[keith-turner]

In #4364 I made some changes to Manager to use Duration and the new NanoTime type. While working on that realized that AccumuloConfiguration.getTimeInMillis() could be a candidate for switching to Duration. Or we could introduce a new AccumuloConfiguration.getDuration() method.

[DomGarguilo]

The changes from #4358, which used Duration in ReadOnlyTStore.unreserve(), were causing issues with MiniAccumuloClusterTest where it was hanging. 23e17129de0350d5496345c78d7fd86080bb1115 reverted those changes. I think replacing ReadOnlyTStore.unreserve() should still be looked into. Need to figure out why it was not working before though.

[DomGarguilo]

In #4364 I made some changes to Manager to use Duration and the new NanoTime type. While working on that realized that AccumuloConfiguration.getTimeInMillis() could be a candidate for switching to Duration. Or we could introduce a new AccumuloConfiguration.getDuration() method.

This seems like a good idea so I started looking into it and realized that all of the places that use this method expect it to be in millis so they use it as such. Initially, we would just be converting from Duration back to millis but I guess that is okay and is the first step to converting things. This made me realize that there are probably a ton of places that could benefit from a conversion to Duration but it would be a big undertaking due to all the places we use millis (or other units) currently.