[GitHub] [lucene] vigyasharma commented on a change in pull request #741: LUCENE-9998: avoid the instant writing rate bigger than the limited rate in merge process

[GitBox <gi...@apache.org>]

vigyasharma commented on a change in pull request #741:
URL: https://github.com/apache/lucene/pull/741#discussion_r824040011



##########
File path: lucene/core/src/java/org/apache/lucene/index/MergeRateLimiter.java
##########
@@ -124,16 +125,21 @@ private long maybePause(long bytes, long curNS) throws MergePolicy.MergeAbortedE
     // Time we should sleep until; this is purely instantaneous
     // rate (just adds seconds onto the last time we had paused to);
     // maybe we should also offer decayed recent history one?
-    long targetNS = lastNS + (long) (1000000000 * secondsToPause);
-
+    long targetNS = lastedTime + (long) (1000000000 * secondsToPause);
     long curPauseNS = targetNS - curNS;

Review comment:
       `lastedTime` is essentially the time time spent between two `pause()` calls, whereas `curNS` is current time. So `curPauseNS` will usually be negative now, right?

##########
File path: lucene/core/src/java/org/apache/lucene/store/RateLimitedIndexOutput.java
##########
@@ -61,42 +64,57 @@ public long getChecksum() throws IOException {
 
   @Override
   public void writeByte(byte b) throws IOException {
+    if (bytesSinceLastPause == 0) {
+      writeStartingTime = System.nanoTime();
+    }
     bytesSinceLastPause++;
-    checkRate();
     delegate.writeByte(b);
+    checkRate();
   }
 
   @Override
   public void writeBytes(byte[] b, int offset, int length) throws IOException {
+    if (bytesSinceLastPause == 0) {
+      writeStartingTime = System.nanoTime();
+    }
     bytesSinceLastPause += length;
-    checkRate();
     delegate.writeBytes(b, offset, length);
+    checkRate();
   }
 
   @Override
   public void writeInt(int i) throws IOException {
+    if (bytesSinceLastPause == 0) {
+      writeStartingTime = System.nanoTime();
+    }
     bytesSinceLastPause += Integer.BYTES;
-    checkRate();
     delegate.writeInt(i);
+    checkRate();
   }
 
   @Override
   public void writeShort(short i) throws IOException {
+    if (bytesSinceLastPause == 0) {
+      writeStartingTime = System.nanoTime();
+    }
     bytesSinceLastPause += Short.BYTES;
-    checkRate();
     delegate.writeShort(i);
+    checkRate();
   }
 
   @Override
   public void writeLong(long i) throws IOException {
+    if (bytesSinceLastPause == 0) {
+      writeStartingTime = System.nanoTime();
+    }
     bytesSinceLastPause += Long.BYTES;
-    checkRate();
     delegate.writeLong(i);
+    checkRate();
   }
 
   private void checkRate() throws IOException {
     if (bytesSinceLastPause > currentMinPauseCheckBytes) {
-      rateLimiter.pause(bytesSinceLastPause);
+      rateLimiter.pause(bytesSinceLastPause, System.nanoTime() - writeStartingTime);

Review comment:
       At a high level, I feel this makes it a little harder to consume a `RateLimiter`, as callers now also have to track some time window. Maybe it is central to solving the instant rate problem, I'm not sure I get it yet. But if not, it would be good to avoid that complexity..
   
   On similar lines, I'm now wondering if `checkRate()` should really be a `RateLimiter` API instead of being implemented by consumers.

##########
File path: lucene/core/src/java/org/apache/lucene/index/MergeRateLimiter.java
##########
@@ -124,16 +125,21 @@ private long maybePause(long bytes, long curNS) throws MergePolicy.MergeAbortedE
     // Time we should sleep until; this is purely instantaneous
     // rate (just adds seconds onto the last time we had paused to);
     // maybe we should also offer decayed recent history one?
-    long targetNS = lastNS + (long) (1000000000 * secondsToPause);
-
+    long targetNS = lastedTime + (long) (1000000000 * secondsToPause);
     long curPauseNS = targetNS - curNS;
 
     // We don't bother with thread pausing if the pause is smaller than 2 msec.
     if (curPauseNS <= MIN_PAUSE_NS) {
-      // Set to curNS, not targetNS, to enforce the instant rate, not
-      // the "averaged over all history" rate:
-      lastNS = curNS;
-      return -1;
+      if (itera == 0) {
+        curPauseNS = (long) (1000000000 * secondsToPause) - lastedTime;

Review comment:
       Since `itera` is always 0 when `maybePause` is called, is the idea to always pause, regardless of when the last pause was called? What is the relevance of `lastedTime` ?
   
   I'm sorry I don't quite understand how this alleviates the instant rate problem. Could you help explain this more?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org