You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "dlmarion (via GitHub)" <gi...@apache.org> on 2024/03/15 12:16:49 UTC
[PR] Resurrected CompactionDriverTest resolving TODO in PreDeleteTable [accumulo]
dlmarion opened a new pull request, #4377:
URL: https://github.com/apache/accumulo/pull/4377
The TODO asked if the the delete marker in ZooKeeper was still being used / tested. It is still being used by the CompactionDriver. However, the CompactionDriverTest class was deleted at some point which tests it. I resurrected and fixed the test.
--
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] Resurrected CompactionDriverTest resolving TODO in PreDeleteTable [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #4377:
URL: https://github.com/apache/accumulo/pull/4377#discussion_r1526643656
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/compact/CompactionDriver.java:
##########
@@ -129,7 +129,8 @@ public long isReady(FateId fateId, Manager manager) throws Exception {
return sleepTime;
}
- private boolean isCancelled(FateId fateId, ServerContext context)
+ // visible for testing
+ protected boolean isCancelled(FateId fateId, ServerContext context)
Review Comment:
```suggestion
// visible for testing
@VisibleForTesting
protected boolean isCancelled(FateId fateId, ServerContext context)
```
Could add this annotation 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
Re: [PR] Resurrected CompactionDriverTest resolving TODO in PreDeleteTable [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #4377:
URL: https://github.com/apache/accumulo/pull/4377#issuecomment-2007594986
CompactionDriverTest was removed in ff5148a8c65d6ffa036e82330fa42378fc5dd9af
--
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] Resurrected CompactionDriverTest resolving TODO in PreDeleteTable [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #4377:
URL: https://github.com/apache/accumulo/pull/4377#issuecomment-2003681565
> > I resurrected and fixed the test.
>
> When was this removed?
I'm not sure. I think it exists in `main`, so it must have been removed as part of development in `elasticity` for some reason.
--
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] Resurrected CompactionDriverTest resolving TODO in PreDeleteTable [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #4377:
URL: https://github.com/apache/accumulo/pull/4377#discussion_r1528467398
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/compact/CompactionDriver.java:
##########
@@ -129,7 +129,8 @@ public long isReady(FateId fateId, Manager manager) throws Exception {
return sleepTime;
}
- private boolean isCancelled(FateId fateId, ServerContext context)
+ // visible for testing
+ protected boolean isCancelled(FateId fateId, ServerContext context)
Review Comment:
We're not using that consistently, and in your suggested change we leave the comment. I'm not sure how this annotation is used other than for documentation purposes, which is the same as a comment. In some cases a comment is more useful as it has details. See:
```
grep -Ri visible * | grep java | grep -i visible
```
--
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] Resurrected CompactionDriverTest resolving TODO in PreDeleteTable [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion merged PR #4377:
URL: https://github.com/apache/accumulo/pull/4377
--
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] Resurrected CompactionDriverTest resolving TODO in PreDeleteTable [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on PR #4377:
URL: https://github.com/apache/accumulo/pull/4377#issuecomment-2000169994
> I resurrected and fixed the test.
When was this removed?
--
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