Add time in index throttle to index stats. #7896

GaelTadh · 2014-09-26T12:13:40Z

This PR adds the time spent throttling indexing to a single thread to the stats API.

mikemccand · 2014-09-26T15:49:39Z

I think docs need to be updated, and we need some sort of test case?

s1monw · 2014-09-26T21:14:10Z

src/main/java/org/elasticsearch/index/engine/internal/InternalEngine.java

@@ -846,6 +851,9 @@ public void flush(Flush flush) throws EngineException {
                        // to be allocated to a different node
                        currentIndexWriter().close(false);
                        indexWriter = createWriter();
+                        if (this.throttle != null) {


honestly this seems just wrong. The throttle is never null. It's designed to be used all the time and it's the actual lock that is used inside class IndexThrottle that make the difference. I think this should be implemented by using a TimedInternalLock extends InternalLock wrapper that is used inside IndexThrottle this implementation actually makes no sense to me to be honest.

there is also no test for this at all I think we need one that 1. checks that is works and 2. that it's 0 if no throttle :)

s1monw · 2014-09-26T21:15:30Z

I left some comments - this PR needs some more work IMO

GaelTadh · 2014-09-29T10:59:10Z

@s1monw I've moved the timing into a TimedLock. @mikemccand we still create a throttle on start and flush. I don't want to change behavior when just adding some timing information. Perhaps a different PR for that.

kimchy · 2014-09-29T11:05:43Z

I was actually thinking of implementing it slightly differently. I was thinking of adding additional callbacks to ShardIndexingService, something like #beforeThrottling and #afterThrottling.

Then, we can keep the relevant stats on ShardIndexingService, and add it to the more appropriate place (compared to SegmentsStats) which is IndexingStats.

kimchy · 2014-09-29T11:08:25Z

also, I think that we should have 2 stats, the first is is_throttled which is true or false, and the second is the time spent throttling.

mikemccand · 2014-09-29T13:35:04Z

I don't want to change behavior when just adding some timing information.

OK makes sense.

GaelTadh · 2014-09-29T14:24:13Z

@kimchy Do you want to add the callbacks at the same place we emit the log message ? I think @s1monw's point is that we are not actually throttling until the throttle lock has been acquired, we are just in a state where we may throttle.

kimchy · 2014-09-30T14:37:15Z

We can put the callback after or before, I personally don't think its as important (missing a millisecond here or there, and now that the method is sync'ed, its simpler to reason). @s1monw what do you think about the idea of callback and adding it to indexing stats?

s1monw · 2014-09-30T14:45:05Z

yeah I agree we can just use a callback - I wonder what info we need here do we want the time we spend indexing under the throttle or the time the index throttle was active? I think these are 2 different impls?

GaelTadh · 2014-10-02T10:15:54Z

@s1monw @kimchy I'm happy to do either one or both ? To be clear the time we spend under the throttle [1] is the time the lock was acquired (the current implementation in this PR) and the time the throttle was active [2] is the time between the two log messages.

For [1] are we ok making a callback from within a lock acquire and release ?

s1monw · 2014-10-14T14:22:03Z

@kimchy can you comment on my latest comment

kimchy · 2014-10-14T14:26:52Z

I think the most important time is the period index throttle was active (much simpler to reason about when comparing it to other rate type graphs)

I would just do the callback when throttling gets enbed, and a callback when it gets disabled.

The indexing stats service can then keep track of the time.

The throttling stats would include the time spent while in throttling mode, and a false/true if it's in throttle mode. Both of those can be easily handled by the callbacks on the indexing stats service

GaelTadh · 2014-10-14T14:28:57Z

Ok sounds good, thanks I'll implement the call back at the time between the two log messages.

GaelTadh · 2014-10-22T11:20:48Z

I've made the changes and added new tests.

s1monw · 2014-10-22T11:56:46Z

src/main/java/org/elasticsearch/index/engine/internal/InternalEngine.java

@@ -1667,7 +1670,7 @@ public void close() throws ElasticsearchException {
        }
    }

-    private static final class InternalLock implements Releasable {


this can be final again?

kimchy · 2014-10-22T11:58:52Z

src/main/java/org/elasticsearch/index/indexing/ShardIndexingService.java

+
+        public void setThrottled(boolean isThrottled) {
+            if (!this.isThrottled && isThrottled) {
+                startOfThrottle = System.nanoTime();


I would use System.currentTimeInMillis, nanoTime has different semantics

s1monw · 2014-10-22T12:08:30Z

left some comments

s1monw · 2014-10-22T12:53:59Z

src/test/java/org/elasticsearch/indices/stats/IndexStatsTests.java

+        // make sure we see throttling kicking in:
+        boolean done = false;
+        while (!done) {
+            for(int i=0;i<100;i++) {


format this loop? as well as the others?

s1monw · 2014-10-22T13:04:32Z

LGTM

This commit adds throttle stats to the indexing stats and uses a call back from InternalEngine to manage the stats. Also includes updates the IndexStatsTests to test for these new stats. Stats added : ``` throttle_time_in_millis is_throttled ``` Closes elastic#7861

GaelTadh added review v1.4.0.Beta1 v2.0.0-beta1 labels Sep 26, 2014

clintongormley added the >enhancement label Sep 26, 2014

s1monw reviewed Sep 26, 2014
View reviewed changes

s1monw removed the review label Sep 26, 2014

GaelTadh added the review label Sep 29, 2014

s1monw added v1.4.0 and removed v1.4.0.Beta1 labels Sep 30, 2014

clintongormley assigned GaelTadh Oct 16, 2014

clintongormley removed the review label Oct 16, 2014

GaelTadh force-pushed the nodestats-indexthrottling branch 2 times, most recently from 7afc1d4 to 8fdbdf6 Compare October 22, 2014 11:19

GaelTadh added the review label Oct 22, 2014

s1monw reviewed Oct 22, 2014
View reviewed changes

kimchy reviewed Oct 22, 2014
View reviewed changes

s1monw removed the review label Oct 22, 2014

GaelTadh added the review label Oct 22, 2014

s1monw reviewed Oct 22, 2014
View reviewed changes

GaelTadh force-pushed the nodestats-indexthrottling branch from 1023a09 to 56940c5 Compare October 22, 2014 13:06

GaelTadh closed this Oct 22, 2014

clintongormley added release highlight and removed review labels Nov 3, 2014

clintongormley added :Data Management/Stats Statistics tracking and retrieval APIs v1.5.0 labels Mar 19, 2015

gmarz mentioned this pull request May 1, 2015

Support for throttle time on indexing stats elastic/elasticsearch-net#1365

Closed

clintongormley changed the title ~~Stats : Add time in index throttle to stats.~~ Add time in index throttle to index stats. Jun 7, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add time in index throttle to index stats. #7896

Add time in index throttle to index stats. #7896

GaelTadh commented Sep 26, 2014

mikemccand commented Sep 26, 2014

s1monw Sep 26, 2014

s1monw Sep 26, 2014

s1monw commented Sep 26, 2014

GaelTadh commented Sep 29, 2014

kimchy commented Sep 29, 2014

kimchy commented Sep 29, 2014

mikemccand commented Sep 29, 2014

GaelTadh commented Sep 29, 2014

kimchy commented Sep 30, 2014

s1monw commented Sep 30, 2014

GaelTadh commented Oct 2, 2014

s1monw commented Oct 14, 2014

kimchy commented Oct 14, 2014

GaelTadh commented Oct 14, 2014

GaelTadh commented Oct 22, 2014

s1monw Oct 22, 2014

kimchy Oct 22, 2014

s1monw commented Oct 22, 2014

s1monw Oct 22, 2014

s1monw commented Oct 22, 2014

Add time in index throttle to index stats. #7896

Add time in index throttle to index stats. #7896

Conversation

GaelTadh commented Sep 26, 2014

mikemccand commented Sep 26, 2014

s1monw Sep 26, 2014

Choose a reason for hiding this comment

s1monw Sep 26, 2014

Choose a reason for hiding this comment

s1monw commented Sep 26, 2014

GaelTadh commented Sep 29, 2014

kimchy commented Sep 29, 2014

kimchy commented Sep 29, 2014

mikemccand commented Sep 29, 2014

GaelTadh commented Sep 29, 2014

kimchy commented Sep 30, 2014

s1monw commented Sep 30, 2014

GaelTadh commented Oct 2, 2014

s1monw commented Oct 14, 2014

kimchy commented Oct 14, 2014

GaelTadh commented Oct 14, 2014

GaelTadh commented Oct 22, 2014

s1monw Oct 22, 2014

Choose a reason for hiding this comment

kimchy Oct 22, 2014

Choose a reason for hiding this comment

s1monw commented Oct 22, 2014

s1monw Oct 22, 2014

Choose a reason for hiding this comment

s1monw commented Oct 22, 2014