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

[GitBox]

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



##########
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:
       `lastedTime` is central if we want to limited instant rate for 
MergeRateLimiter, if we can tolerate potential large instant rate write bursts, 
we can don't use the `lastedTime`(we set it always to be 0 ), just like 
`SimpleRateLimiter` what to do. whether it‘s complexity depends on your 
tolerate. 
   
   ·RateLimiter· is only for speed rate, consumer determines whether rate limit 
is required, it seems all right.




-- 
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-unsubscr...@lucene.apache.org

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



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