You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "keith-turner (via GitHub)" <gi...@apache.org> on 2023/07/10 17:58:09 UTC
[GitHub] [accumulo] keith-turner opened a new pull request, #3582: adds logging when tablet close is waiting on a scan
keith-turner opened a new pull request, #3582:
URL: https://github.com/apache/accumulo/pull/3582
this is related to #3512
--
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] keith-turner commented on a diff in pull request #3582: adds logging when tablet close is waiting on a scan
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3582:
URL: https://github.com/apache/accumulo/pull/3582#discussion_r1259946034
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java:
##########
@@ -978,8 +979,20 @@ synchronized void completeClose(boolean saveState, boolean completeClose) throws
activeScan.interrupt();
}
+ long lastLogTime = System.nanoTime();
+
// wait for reads and writes to complete
while (writesInProgress > 0 || !activeScans.isEmpty()) {
+
+ if (log.isDebugEnabled()
+ && System.nanoTime() - lastLogTime > Duration.ofSeconds(60).toNanos()) {
Review Comment:
Updated in 65193af to use TimeUnit and I think that looks good. I did not do the static import for two reasons. First the if stmt was shortened using TimeUnit and fit on a single line w/o doing the static import. Second its the only use of TimeUnit.SECONDS in the class, so did not want to add a static import for a single use.
--
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] ctubbsii commented on a diff in pull request #3582: adds logging when tablet close is waiting on a scan
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3582:
URL: https://github.com/apache/accumulo/pull/3582#discussion_r1259213856
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java:
##########
@@ -978,8 +979,20 @@ synchronized void completeClose(boolean saveState, boolean completeClose) throws
activeScan.interrupt();
}
+ long lastLogTime = System.nanoTime();
+
// wait for reads and writes to complete
while (writesInProgress > 0 || !activeScans.isEmpty()) {
+
+ if (log.isDebugEnabled()
+ && System.nanoTime() - lastLogTime > Duration.ofSeconds(60).toNanos()) {
Review Comment:
Duration.ofSeconds() is overkill, as it would create wrapper objects just to do a bit of math. It's much better to just use `SECONDS.toNanos(60)` (static import TimeUnit.SECONDS).
--
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] keith-turner commented on a diff in pull request #3582: adds logging when tablet close is waiting on a scan
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3582:
URL: https://github.com/apache/accumulo/pull/3582#discussion_r1259932809
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java:
##########
@@ -978,8 +979,20 @@ synchronized void completeClose(boolean saveState, boolean completeClose) throws
activeScan.interrupt();
}
+ long lastLogTime = System.nanoTime();
+
// wait for reads and writes to complete
while (writesInProgress > 0 || !activeScans.isEmpty()) {
+
+ if (log.isDebugEnabled()
+ && System.nanoTime() - lastLogTime > Duration.ofSeconds(60).toNanos()) {
Review Comment:
I look into that and see what it looks like. I wrote that code intentionally like that thinking it was more readable and that in this particular case performance did not matter because its only called when debug log is enabled and its in a loop that sleeps (a bit later in the loop it sleeps 50ms).
--
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 diff in pull request #3582: adds logging when tablet close is waiting on a scan
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #3582:
URL: https://github.com/apache/accumulo/pull/3582#discussion_r1260091082
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java:
##########
@@ -286,4 +286,22 @@ public void setInterruptFlag(AtomicBoolean flag) {
throw new UnsupportedOperationException();
}
+ @Override
+ public String toString() {
+ StringBuilder sb = new StringBuilder();
+ sb.append("isNull(memIters):");
+ sb.append(memIters == null);
+ sb.append(" isNull(fileManager):");
+ sb.append(fileManager == null);
+ sb.append(" fileReservationId:");
+ sb.append(fileReservationId);
+ sb.append(" interruptFlag:");
+ sb.append(interruptFlag.get());
+ sb.append(" expectedDeletionCount:");
+ sb.append(expectedDeletionCount);
+ sb.append(" scanParams=[");
+ sb.append(scanParams);
+ sb.append("]");
+ return sb.toString();
+ }
Review Comment:
```suggestion
@Override
public String toString() {
return new ToStringBuilder(this, ToStringStyle.SHORT_PREFIX_STYLE)
.append("isNull(memIters)", memIters == null)
.append("isNull(fileManager)", fileManager == null)
.append("fileReservationId", fileReservationId)
.append("interruptFlag", interruptFlag.get())
.append("expectedDeletionCount", expectedDeletionCount)
.append("scanParams", scanParams)
.toString();
}
```
Not a huge difference, but could explore the use of `ToStringBuilder` to clean things up here.
--
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] keith-turner merged pull request #3582: adds logging when tablet close is waiting on a scan
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner merged PR #3582:
URL: https://github.com/apache/accumulo/pull/3582
--
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