Replace superfluous usage of Counter with Supplier by matriv · Pull Request #39048 · elastic/elasticsearch

matriv · 2019-02-18T10:58:13Z

Counter was used as a means of a functional argument to pass
around the relative cached time before Supplier iface was introduced.

`Counter` was used as a means of a functional argument to pass the relative cached time before `Supplier` iface was introduced.

elasticmachine · 2019-02-18T10:58:16Z

Pinging @elastic/es-core-infra

s1monw

left some comments

s1monw · 2019-02-18T11:40:36Z

server/src/main/java/org/elasticsearch/search/DefaultSearchContext.java

    private final ShardSearchRequest request;
    private final SearchShardTarget shardTarget;
-    private final Counter timeEstimateCounter;
+    private final Supplier<Long> timeEstimate;


use LongSupplier instead

s1monw · 2019-02-18T11:42:07Z

server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java


-    public Counter estimatedTimeInMillisCounter() {
-        return cachedTimeThread.counter;
+    public Supplier<Long> estimatedTimeInMillis() {


this can be removed and replaced by ThreadPool::relativeTimeInMillis on the consumer end

s1monw · 2019-02-18T11:42:24Z

server/src/main/java/org/elasticsearch/search/SearchService.java


        final DefaultSearchContext searchContext = new DefaultSearchContext(idGenerator.incrementAndGet(), request, shardTarget,
-            engineSearcher, clusterService, indexService, indexShard, bigArrays, threadPool.estimatedTimeInMillisCounter(), timeout,
+            engineSearcher, clusterService, indexService, indexShard, bigArrays, threadPool.estimatedTimeInMillis(), timeout,


use threadPool::relativeTimeInMillis

s1monw · 2019-02-18T11:43:09Z

test/framework/src/main/java/org/elasticsearch/cluster/coordination/DeterministicTaskQueue.java

-                        return currentTimeMillis;
-                    }
-                };
+            public Supplier<Long> estimatedTimeInMillis() {


just use :: relativeTimeInMillis on the consumer end.

matriv · 2019-02-18T12:10:11Z

@s1monw Thank you! Addressed comments.

jasontedor

I left a comment about naming.

jasontedor · 2019-02-18T15:18:53Z

server/src/main/java/org/elasticsearch/search/DefaultSearchContext.java

    private final ShardSearchRequest request;
    private final SearchShardTarget shardTarget;
-    private final Counter timeEstimateCounter;
+    private final LongSupplier timeEstimate;


I think this name is misleading because we are using a relative notion of time here whereas the name could be construed as an estimate of the absolute time (like a timestamp). In other places we use the name relativeTimeSupplier to make it clear the purpose of the variable, and to avoid a problem if in the future someone were to use this variable for another piece of code to try to get the absolute time. It would also be worthwhile to add a comment explaining why relative time is okay here. It is important to keep the two uses between absolute and relative time absolutely clear.

Should we change also the iface method name(): https://github.com/elastic/elasticsearch/pull/39048/files#diff-2410736cbe252badfd50ccbfceebc857R398 ?

I would just name it timeSupplier. I also think we should not expose the LongSupplier on SerachContext at all and only provide a method to access the time public long getTime() on SearchContext.java The usage is simple and can just call this method. no need to expose the supplier.

Chose getTimeInMillis() to be even more clear.

Please include the word relative in the name.

s1monw

left one more comment

s1monw · 2019-02-19T11:28:02Z

server/src/main/java/org/elasticsearch/search/DefaultSearchContext.java

    private final ShardSearchRequest request;
    private final SearchShardTarget shardTarget;
-    private final Counter timeEstimateCounter;
+    private final LongSupplier timeEstimate;


I would just name it timeSupplier. I also think we should not expose the LongSupplier on SerachContext at all and only provide a method to access the time public long getTime() on SearchContext.java The usage is simple and can just call this method. no need to expose the supplier.

s1monw

LGTM

jasontedor

I still have concerns about the naming not being explicit enough.

matriv · 2019-02-20T16:45:08Z

@jasontedor please check again

jasontedor

LGTM. Thank you for the improvement and all the iterations.

`Counter` was used as a means of a functional argument to pass the relative cached time before `Supplier` iface was introduced.

* elastic/master: Ensure index commit released when testing timeouts (elastic#39273) Avoid using TimeWarp in TransformIntegrationTests. (elastic#39277) Fixed missed stopping of SchedulerEngine (elastic#39193) [CI] Mute CcrRetentionLeaseIT.testRetentionLeaseIsRenewedDuringRecovery (elastic#39269) Muting AutoFollowIT.testAutoFollowManyIndices (elastic#39264) Clarify the use of sleep in CCR test Fix testCannotShrinkLeaderIndex (elastic#38529) Fix CCR tests that manipulate transport requests Align generated release notes with doc standards (elastic#39234) Mute test (elastic#39248) ReadOnlyEngine should update translog recovery state information (elastic#39238) Wrap accounting breaker check in assertBusy (elastic#39211) Simplify and Fix Synchronization in InternalTestCluster (elastic#39168) [Tests] Make testEngineGCDeletesSetting deterministic (elastic#38942) Extend nextDoc to delegate to the wrapped doc-value iterator for date_nanos (elastic#39176) Change ShardFollowTask to reuse common serialization logic (elastic#39094) Replace superfluous usage of Counter with Supplier (elastic#39048) Disable bwc tests for elastic#39094

`Counter` was used as a means of a functional argument to pass the relative cached time before `Supplier` iface was introduced.

Replace superfluous Counter with Supplier

8339c2a

`Counter` was used as a means of a functional argument to pass the relative cached time before `Supplier` iface was introduced.

matriv added >non-issue :Core/Infra/Core Core issues without another label v8.0.0 v7.2.0 labels Feb 18, 2019

matriv requested a review from s1monw February 18, 2019 10:58

matriv changed the title ~~Replace superfluous Counter with Supplier~~ Replace superfluous usage of Counter with Supplier Feb 18, 2019

Fix subclasses and rename method

934bf9a

s1monw suggested changes Feb 18, 2019

View reviewed changes

Address comments

45b1afa

jasontedor reviewed Feb 18, 2019

View reviewed changes

s1monw suggested changes Feb 19, 2019

View reviewed changes

Address comments

260c320

matriv requested a review from s1monw February 19, 2019 13:14

s1monw approved these changes Feb 19, 2019

View reviewed changes

jasontedor requested changes Feb 19, 2019

View reviewed changes

Add 'relative' to variable and method name for more clarity

40b56d0

matriv requested a review from jasontedor February 19, 2019 21:24

jasontedor approved these changes Feb 20, 2019

View reviewed changes

matriv merged commit 8eb649c into elastic:master Feb 21, 2019

matriv deleted the mt/replace-counter branch February 21, 2019 08:23

matriv added the backport pending label Feb 21, 2019

matriv added a commit that referenced this pull request Feb 21, 2019

Replace superfluous usage of Counter with Supplier (#39048) (#39225)

1316825

`Counter` was used as a means of a functional argument to pass the relative cached time before `Supplier` iface was introduced.

matriv removed the backport pending label Feb 21, 2019

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Conversation

matriv commented Feb 18, 2019

Uh oh!

elasticmachine commented Feb 18, 2019

Uh oh!

s1monw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matriv commented Feb 18, 2019

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s1monw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s1monw left a comment

Choose a reason for hiding this comment

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

matriv commented Feb 20, 2019

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants