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/02/18 16:14:15 UTC
[GitHub] [accumulo] milleruntime opened a new pull request #1937: Add compaction stats to new Compactable classes
milleruntime opened a new pull request #1937:
URL: https://github.com/apache/accumulo/pull/1937
The stats were missing from the Monitor. See #1930
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime merged pull request #1937: Add stats to new Compactable classes. Fixes #1930
Posted by GitBox <gi...@apache.org>.
milleruntime merged pull request #1937:
URL: https://github.com/apache/accumulo/pull/1937
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #1937: Add stats to new Compactable classes. Fixes #1930
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1937:
URL: https://github.com/apache/accumulo/pull/1937#discussion_r578787592
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableImpl.java
##########
@@ -679,6 +688,8 @@ public void compact(CompactionServiceId service, CompactionJob job, RateLimiter
selectedCompactionCompleted(job, jobFiles, metaFile);
else
selectFiles();
+
+ tablet.updateTimer(MAJOR, queuedTime, startTime, stats.getEntriesRead(), failed);
Review comment:
Could shorten the code a bit by removing failed boolean. Please ignore this suggestion if it does not suit you. Just something I thought of when I looked at the code.
```suggestion
tablet.updateTimer(MAJOR, queuedTime, startTime, stats.getEntriesRead(), metaFile == null);
```
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableImpl.java
##########
@@ -641,13 +643,18 @@ public void compact(CompactionServiceId service, CompactionJob job, RateLimiter
}
StoredTabletFile metaFile = null;
+ long startTime = System.currentTimeMillis();
+ long entriesWritten = 0;
+ boolean failed = false;
+ CompactionStats stats = new CompactionStats();
try {
TabletLogger.compacting(getExtent(), job, localCompactionCfg);
+ tablet.incrementStatusMajor();
Review comment:
What does this method do? If its nots quick to explain just let me know, I can look at the source.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #1937: Add stats to new Compactable classes. Fixes #1930
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1937:
URL: https://github.com/apache/accumulo/pull/1937#discussion_r579184201
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableImpl.java
##########
@@ -679,6 +688,8 @@ public void compact(CompactionServiceId service, CompactionJob job, RateLimiter
selectedCompactionCompleted(job, jobFiles, metaFile);
else
selectFiles();
+
+ tablet.updateTimer(MAJOR, queuedTime, startTime, stats.getEntriesRead(), failed);
Review comment:
I like it. As long as it doesn't go over the char line limit... *drum roll*
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #1937: Add stats to new Compactable classes. Fixes #1930
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1937:
URL: https://github.com/apache/accumulo/pull/1937#discussion_r579192468
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableImpl.java
##########
@@ -679,6 +688,8 @@ public void compact(CompactionServiceId service, CompactionJob job, RateLimiter
selectedCompactionCompleted(job, jobFiles, metaFile);
else
selectFiles();
+
+ tablet.updateTimer(MAJOR, queuedTime, startTime, stats.getEntriesRead(), failed);
Review comment:
98 chars!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #1937: Add stats to new Compactable classes. Fixes #1930
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #1937:
URL: https://github.com/apache/accumulo/pull/1937#issuecomment-781643735
My only concern with this fix was that it modifies the new `Compactable` interface, by adding a param to the `compact()` method. @keith-turner any thoughts?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #1937: Add stats to new Compactable classes. Fixes #1930
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1937:
URL: https://github.com/apache/accumulo/pull/1937#discussion_r579181855
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableImpl.java
##########
@@ -641,13 +643,18 @@ public void compact(CompactionServiceId service, CompactionJob job, RateLimiter
}
StoredTabletFile metaFile = null;
+ long startTime = System.currentTimeMillis();
+ long entriesWritten = 0;
+ boolean failed = false;
+ CompactionStats stats = new CompactionStats();
try {
TabletLogger.compacting(getExtent(), job, localCompactionCfg);
+ tablet.incrementStatusMajor();
Review comment:
It increments the counter for ongoing major compactions. If you are wondering about synchronization, there is none. The stats are stored in a thrift object called `ActionStats`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #1937: Add stats to new Compactable classes. Fixes #1930
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #1937:
URL: https://github.com/apache/accumulo/pull/1937#issuecomment-781525996
This will also fix the splits in the monitor.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1937: Add stats to new Compactable classes. Fixes #1930
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1937:
URL: https://github.com/apache/accumulo/pull/1937#issuecomment-781672952
> My only concern with this fix was that it modifies the new `Compactable` interface
But, that's not public API or SPI, so it shouldn't matter.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org