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 2022/07/11 13:00:46 UTC
[GitHub] [accumulo] dlmarion opened a new pull request, #2804: Modify CompactionCheck logic to run expensive checks less often
dlmarion opened a new pull request, #2804:
URL: https://github.com/apache/accumulo/pull/2804
This change splits the prior memoized check into two, one that
is less expensive and is checked more often and one that is more
expensive, likely to change less often and is checked less often
Closes #1610
--
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 diff in pull request #2804: Modify CompactionCheck logic to run expensive checks less often
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2804:
URL: https://github.com/apache/accumulo/pull/2804#discussion_r926888990
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableImpl.java:
##########
@@ -1105,23 +1105,23 @@ public Optional<Files> getFiles(CompactionServiceId service, CompactionKind kind
}
class CompactionCheck {
- private final Supplier<Boolean> memoizedCheck;
+ private final Supplier<Boolean> expensiveCheck;
+ private final Supplier<Boolean> inexpensiveCheck;
public CompactionCheck(CompactionServiceId service, CompactionKind kind, Long compactionId) {
- this.memoizedCheck = Suppliers.memoizeWithExpiration(() -> {
- if (closed)
+ this.expensiveCheck = Suppliers.memoizeWithExpiration(() -> {
+ return service.equals(getConfiguredService(kind));
Review Comment:
maybe `expensive` was the wrong term, the more expensive check in this case is more complex than comparing enums and longs. The more expensive check evaluates whether the service parameter is equal to the configured service, meaning, has the configuration changed. The path to getting there is more complex.
--
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 #2804: Modify CompactionCheck logic to run expensive checks less often
Posted by GitBox <gi...@apache.org>.
dlmarion merged PR #2804:
URL: https://github.com/apache/accumulo/pull/2804
--
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 #2804: Modify CompactionCheck logic to run expensive checks less often
Posted by GitBox <gi...@apache.org>.
keith-turner commented on code in PR #2804:
URL: https://github.com/apache/accumulo/pull/2804#discussion_r922301056
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableImpl.java:
##########
@@ -1105,23 +1105,23 @@ public Optional<Files> getFiles(CompactionServiceId service, CompactionKind kind
}
class CompactionCheck {
- private final Supplier<Boolean> memoizedCheck;
+ private final Supplier<Boolean> expensiveCheck;
+ private final Supplier<Boolean> inexpensiveCheck;
public CompactionCheck(CompactionServiceId service, CompactionKind kind, Long compactionId) {
- this.memoizedCheck = Suppliers.memoizeWithExpiration(() -> {
- if (closed)
+ this.expensiveCheck = Suppliers.memoizeWithExpiration(() -> {
+ return service.equals(getConfiguredService(kind));
+ }, 3, TimeUnit.SECONDS);
+ this.inexpensiveCheck = Suppliers.memoizeWithExpiration(() -> {
+ if (closed
+ || (kind == CompactionKind.USER && lastSeenCompactionCancelId.get() >= compactionId))
return false;
- if (!service.equals(getConfiguredService(kind)))
- return false;
- if (kind == CompactionKind.USER && lastSeenCompactionCancelId.get() >= compactionId)
- return false;
-
return true;
- }, 100, TimeUnit.MILLISECONDS);
+ }, 50, TimeUnit.MILLISECONDS);
Review Comment:
Was there a specific reason to adjust this time from 100 to 50? The initial 100ms time was arbitrarily chosen, so not in any way opposed to changing it. Just curious if you saw something that prompted this.
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableImpl.java:
##########
@@ -1105,23 +1105,23 @@ public Optional<Files> getFiles(CompactionServiceId service, CompactionKind kind
}
class CompactionCheck {
- private final Supplier<Boolean> memoizedCheck;
+ private final Supplier<Boolean> expensiveCheck;
+ private final Supplier<Boolean> inexpensiveCheck;
public CompactionCheck(CompactionServiceId service, CompactionKind kind, Long compactionId) {
- this.memoizedCheck = Suppliers.memoizeWithExpiration(() -> {
- if (closed)
+ this.expensiveCheck = Suppliers.memoizeWithExpiration(() -> {
+ return service.equals(getConfiguredService(kind));
Review Comment:
How was it determined this check was expensive?
--
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 diff in pull request #2804: Modify CompactionCheck logic to run expensive checks less often
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2804:
URL: https://github.com/apache/accumulo/pull/2804#discussion_r926890612
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableImpl.java:
##########
@@ -1105,23 +1105,23 @@ public Optional<Files> getFiles(CompactionServiceId service, CompactionKind kind
}
class CompactionCheck {
- private final Supplier<Boolean> memoizedCheck;
+ private final Supplier<Boolean> expensiveCheck;
+ private final Supplier<Boolean> inexpensiveCheck;
public CompactionCheck(CompactionServiceId service, CompactionKind kind, Long compactionId) {
- this.memoizedCheck = Suppliers.memoizeWithExpiration(() -> {
- if (closed)
+ this.expensiveCheck = Suppliers.memoizeWithExpiration(() -> {
+ return service.equals(getConfiguredService(kind));
+ }, 3, TimeUnit.SECONDS);
+ this.inexpensiveCheck = Suppliers.memoizeWithExpiration(() -> {
+ if (closed
+ || (kind == CompactionKind.USER && lastSeenCompactionCancelId.get() >= compactionId))
return false;
- if (!service.equals(getConfiguredService(kind)))
- return false;
- if (kind == CompactionKind.USER && lastSeenCompactionCancelId.get() >= compactionId)
- return false;
-
return true;
- }, 100, TimeUnit.MILLISECONDS);
+ }, 50, TimeUnit.MILLISECONDS);
Review Comment:
No reason. I figured that since the less and more complex checks were separated, that they could run at different intervals. I figured it made sense to lower the less complex check interval so that compactions could be cancelled a little faster if needed. This too was arbitrary, I could have left it at 100
--
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