You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "kevinrr888 (via GitHub)" <gi...@apache.org> on 2024/07/08 19:46:42 UTC
[PR] Fixed bug with USER compaction hints showing on SYSTEM compactions [accumulo]
kevinrr888 opened a new pull request, #4730:
URL: https://github.com/apache/accumulo/pull/4730
Closes #4460
- Confirmed that these changes fix the issue: no longer shows hints on SYSTEM compactions which follow a USER compaction that had hints
- This may not be the best way to fix this issue but this keeps the amount of code changes small. Figured filtering them out at the time they are logged may not be the best approach in case these incorrect hints would be seen/used deeper in the code. But also didn't filter them out at the deepest level where they are set. This would have required larger refactoring.
- This issue made me think that maybe these execution hints would carry over to new USER compactions. For example, if a user compaction is initiated with execution hints "hints1", and later another user compaction was initiated with no execution hints, we may still see "hints1". I confirmed that this is not the case.
--
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
Re: [PR] Fixed bug with USER compaction hints showing on SYSTEM compactions [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4730:
URL: https://github.com/apache/accumulo/pull/4730#discussion_r1670776452
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableImpl.java:
##########
@@ -1281,6 +1281,10 @@ private Optional<CompactionInfo> reserveFilesForCompaction(CompactionServiceId s
cInfo.localHelper = this.chelper;
cInfo.localCompactionCfg = this.compactionConfig;
+ // don't include execution hints, which may have been previously set, for system compactions
+ if (job.getKind() == CompactionKind.SYSTEM) {
+ cInfo.localCompactionCfg.setExecutionHints(Collections.emptyMap());
Review Comment:
`cInfo.localCompactionCfg` is a reference to `this.compactionConfig` so changing it will change `this.compactionConfig` which could cause bugs with user compactions.
`cInfo.localCompactionCfg` really only needs to be logged for user compactions. One way to achieve this is to only set `cInfo.localCompactionCfg` for user compactions and have it be null otherwise. This would log null for the config then.
--
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
Re: [PR] Fixed bug with USER compaction hints showing on SYSTEM compactions [accumulo]
Posted by "kevinrr888 (via GitHub)" <gi...@apache.org>.
kevinrr888 commented on code in PR #4730:
URL: https://github.com/apache/accumulo/pull/4730#discussion_r1670816224
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableImpl.java:
##########
@@ -1281,6 +1281,10 @@ private Optional<CompactionInfo> reserveFilesForCompaction(CompactionServiceId s
cInfo.localHelper = this.chelper;
cInfo.localCompactionCfg = this.compactionConfig;
+ // don't include execution hints, which may have been previously set, for system compactions
+ if (job.getKind() == CompactionKind.SYSTEM) {
+ cInfo.localCompactionCfg.setExecutionHints(Collections.emptyMap());
Review Comment:
Ah okay wasn't sure if other config was still needed in the logs for SYSTEM compactions. Addressed
--
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
Re: [PR] Fixed bug with USER compaction hints showing on SYSTEM compactions [accumulo]
Posted by "kevinrr888 (via GitHub)" <gi...@apache.org>.
kevinrr888 commented on code in PR #4730:
URL: https://github.com/apache/accumulo/pull/4730#discussion_r1670818774
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableImpl.java:
##########
@@ -1281,6 +1281,10 @@ private Optional<CompactionInfo> reserveFilesForCompaction(CompactionServiceId s
cInfo.localHelper = this.chelper;
cInfo.localCompactionCfg = this.compactionConfig;
+ // don't include execution hints, which may have been previously set, for system compactions
+ if (job.getKind() == CompactionKind.SYSTEM) {
+ cInfo.localCompactionCfg.setExecutionHints(Collections.emptyMap());
Review Comment:
Also manually verified that this change works
--
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
Re: [PR] Fixed bug with USER compaction hints showing on SYSTEM compactions [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4730:
URL: https://github.com/apache/accumulo/pull/4730#discussion_r1670776452
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableImpl.java:
##########
@@ -1281,6 +1281,10 @@ private Optional<CompactionInfo> reserveFilesForCompaction(CompactionServiceId s
cInfo.localHelper = this.chelper;
cInfo.localCompactionCfg = this.compactionConfig;
+ // don't include execution hints, which may have been previously set, for system compactions
+ if (job.getKind() == CompactionKind.SYSTEM) {
+ cInfo.localCompactionCfg.setExecutionHints(Collections.emptyMap());
Review Comment:
`cInfo.localCompactionCfg` is a reference to `this.compactionConfig` so changing it will change `this.compactionConfig` which could cause bugs with user compactions.
`cInfo.localCompactionCfg` really only needs to be logged for user compactions. One way to achieve this is to only set `cInfo.localCompactionCfg` for user compactions and have it be null otherwise. This would log null for the config for non user compactions
--
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
Re: [PR] Fixed bug with USER compaction hints showing on SYSTEM compactions [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner merged PR #4730:
URL: https://github.com/apache/accumulo/pull/4730
--
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