You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2021/12/20 18:53:57 UTC
[GitHub] [accumulo] dlmarion opened a new pull request #2396: Add metric for number of open files for scans
dlmarion opened a new pull request #2396:
URL: https://github.com/apache/accumulo/pull/2396
Closes #2192
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2396: Add metric for number of open files for scans
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2396:
URL: https://github.com/apache/accumulo/pull/2396#discussion_r772667394
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/metrics/TabletServerScanMetrics.java
##########
@@ -44,8 +47,18 @@ public void addYield(long value) {
yields.record(value);
}
+ public void incrementOpenFiles(int numOpenFiles) {
+ openFiles.addAndGet(numOpenFiles);
+ }
+
+ public void decrementOpenFiles(int numOpenFiles) {
Review comment:
See 4b0db48
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2396: Add metric for number of open files for scans
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2396:
URL: https://github.com/apache/accumulo/pull/2396#discussion_r773209128
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/metrics/TabletServerScanMetrics.java
##########
@@ -44,8 +49,19 @@ public void addYield(long value) {
yields.record(value);
}
+ public void incrementOpenFiles(int delta) {
+ Preconditions.checkArgument(delta >= 0, "Parameter must be zero or positive");
Review comment:
I like this better, changed in b1fb539
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2396: Add metric for number of open files for scans
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2396:
URL: https://github.com/apache/accumulo/pull/2396#discussion_r772652455
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/metrics/TabletServerScanMetrics.java
##########
@@ -44,8 +47,18 @@ public void addYield(long value) {
yields.record(value);
}
+ public void incrementOpenFiles(int numOpenFiles) {
+ openFiles.addAndGet(numOpenFiles);
+ }
+
+ public void decrementOpenFiles(int numOpenFiles) {
Review comment:
Good catch. I don't think its possible that the fileManager will return a negative number though since it is just giving the size of the ArrayList. You could put assert checks at the beginning of each method to make sure the parameter is positive, then always multiplying by -1 for the decrementing.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a change in pull request #2396: Add metric for number of open files for scans
Posted by GitBox <gi...@apache.org>.
EdColeman commented on a change in pull request #2396:
URL: https://github.com/apache/accumulo/pull/2396#discussion_r773154732
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/metrics/TabletServerScanMetrics.java
##########
@@ -44,8 +49,19 @@ public void addYield(long value) {
yields.record(value);
}
+ public void incrementOpenFiles(int delta) {
+ Preconditions.checkArgument(delta >= 0, "Parameter must be zero or positive");
Review comment:
Do we need to throw an unchecked exception here? Would Math.max(0, val) work instead?
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2396: Add metric for number of open files for scans
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2396:
URL: https://github.com/apache/accumulo/pull/2396#discussion_r772652455
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/metrics/TabletServerScanMetrics.java
##########
@@ -44,8 +47,18 @@ public void addYield(long value) {
yields.record(value);
}
+ public void incrementOpenFiles(int numOpenFiles) {
+ openFiles.addAndGet(numOpenFiles);
+ }
+
+ public void decrementOpenFiles(int numOpenFiles) {
Review comment:
Good catch. I don't think its possible that the fileManager will return a negative number though since it is just giving the size of the ArrayList. You could put assert checks at the beginning of each method to make sure the parameter is positive, then always multiply by -1 for the decrementing.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2396: Add metric for number of open files for scans
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2396:
URL: https://github.com/apache/accumulo/pull/2396#discussion_r772623779
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/metrics/TabletServerScanMetrics.java
##########
@@ -44,8 +47,18 @@ public void addYield(long value) {
yields.record(value);
}
+ public void incrementOpenFiles(int numOpenFiles) {
+ openFiles.addAndGet(numOpenFiles);
+ }
+
+ public void decrementOpenFiles(int numOpenFiles) {
Review comment:
LOL - yes, in my mind, that's what was happening. I should probably multiple the argument by -1. Good catch.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a change in pull request #2396: Add metric for number of open files for scans
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2396:
URL: https://github.com/apache/accumulo/pull/2396#discussion_r772622450
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/metrics/TabletServerScanMetrics.java
##########
@@ -44,8 +47,18 @@ public void addYield(long value) {
yields.record(value);
}
+ public void incrementOpenFiles(int numOpenFiles) {
+ openFiles.addAndGet(numOpenFiles);
+ }
+
+ public void decrementOpenFiles(int numOpenFiles) {
Review comment:
In the cases where this is used, will the value of `openFiles` ever actually be decremented? It looks like `incrementOpenFiles()` and `decrementOpenFiles()` will both do the same thing at the moment. Is the idea that we always pass a negative number to `decrementOpenFiles()`?
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a change in pull request #2396: Add metric for number of open files for scans
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2396:
URL: https://github.com/apache/accumulo/pull/2396#discussion_r772636870
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/metrics/TabletServerScanMetrics.java
##########
@@ -52,7 +52,7 @@ public void incrementOpenFiles(int numOpenFiles) {
}
public void decrementOpenFiles(int numOpenFiles) {
- openFiles.addAndGet(numOpenFiles);
+ openFiles.addAndGet(numOpenFiles > 0 ? numOpenFiles : numOpenFiles * -1);
Review comment:
So with this change, if the number passed to `decrementOpenFiles()` is positive it will be added to `openFiles` else if its negative, it will be made positive (multiplied by -1) and added to `openFiles`?
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion merged pull request #2396: Add metric for number of open files for scans
Posted by GitBox <gi...@apache.org>.
dlmarion merged pull request #2396:
URL: https://github.com/apache/accumulo/pull/2396
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a change in pull request #2396: Add metric for number of open files for scans
Posted by GitBox <gi...@apache.org>.
EdColeman commented on a change in pull request #2396:
URL: https://github.com/apache/accumulo/pull/2396#discussion_r773233318
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/metrics/TabletServerScanMetrics.java
##########
@@ -49,7 +49,7 @@ public void addYield(long value) {
public void incrementOpenFiles(int delta) {
Math.max(0, delta);
Review comment:
Without assignment, this statement is unnecessary. Sorry - I didn't make it a multi-edit suggestion.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2396: Add metric for number of open files for scans
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2396:
URL: https://github.com/apache/accumulo/pull/2396#discussion_r772640788
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/metrics/TabletServerScanMetrics.java
##########
@@ -52,7 +52,7 @@ public void incrementOpenFiles(int numOpenFiles) {
}
public void decrementOpenFiles(int numOpenFiles) {
- openFiles.addAndGet(numOpenFiles);
+ openFiles.addAndGet(numOpenFiles > 0 ? numOpenFiles : numOpenFiles * -1);
Review comment:
What you are saying is that I'm doing a really bad job of multi-tasking today. :-)
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a change in pull request #2396: Add metric for number of open files for scans
Posted by GitBox <gi...@apache.org>.
EdColeman commented on a change in pull request #2396:
URL: https://github.com/apache/accumulo/pull/2396#discussion_r773236989
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/metrics/TabletServerScanMetrics.java
##########
@@ -49,7 +49,7 @@ public void addYield(long value) {
public void incrementOpenFiles(int delta) {
Math.max(0, delta);
Review comment:
Or - more coffee
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a change in pull request #2396: Add metric for number of open files for scans
Posted by GitBox <gi...@apache.org>.
EdColeman commented on a change in pull request #2396:
URL: https://github.com/apache/accumulo/pull/2396#discussion_r773228429
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/metrics/TabletServerScanMetrics.java
##########
@@ -44,8 +47,19 @@ public void addYield(long value) {
yields.record(value);
}
+ public void incrementOpenFiles(int delta) {
+ Math.max(0, delta);
+ openFiles.addAndGet(delta);
Review comment:
```suggestion
openFiles.addAndGet(Math.max(0,delta));
```
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2396: Add metric for number of open files for scans
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2396:
URL: https://github.com/apache/accumulo/pull/2396#discussion_r773235407
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/metrics/TabletServerScanMetrics.java
##########
@@ -49,7 +49,7 @@ public void addYield(long value) {
public void incrementOpenFiles(int delta) {
Math.max(0, delta);
Review comment:
No, you're right, good catch. I've made like 4 mistakes on this simple change. I think I just need to start my vacation now.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2396: Add metric for number of open files for scans
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2396:
URL: https://github.com/apache/accumulo/pull/2396#discussion_r772654539
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/metrics/TabletServerScanMetrics.java
##########
@@ -44,8 +47,18 @@ public void addYield(long value) {
yields.record(value);
}
+ public void incrementOpenFiles(int numOpenFiles) {
+ openFiles.addAndGet(numOpenFiles);
+ }
+
+ public void decrementOpenFiles(int numOpenFiles) {
Review comment:
Otherwise, the parameter name is odd with the way you have it now since you can't have negative "numOpenFiles". If you want to allow passing a negative parameter you could name it "deltaOpenFiles" or something similar since the parameter to `addAndGet` is called "delta". https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/concurrent/atomic/AtomicInteger.html#addAndGet(int)
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a change in pull request #2396: Add metric for number of open files for scans
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2396:
URL: https://github.com/apache/accumulo/pull/2396#discussion_r772658677
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/metrics/TabletServerScanMetrics.java
##########
@@ -44,8 +47,18 @@ public void addYield(long value) {
yields.record(value);
}
+ public void incrementOpenFiles(int numOpenFiles) {
+ openFiles.addAndGet(numOpenFiles);
+ }
+
+ public void decrementOpenFiles(int numOpenFiles) {
Review comment:
I agree, I think it makes more sense to either:
keep both methods and only allowing non-negative numbers for each, multiplying by -1 for decrement()
**or,**
just use one method that will add to `openFiles` regardless of whether the input is positive or negative. (preserving the functionality of `addAndGet()`)
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org