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/09/10 13:27:21 UTC
[GitHub] [accumulo] cshannon opened a new pull request, #2925: Adding CleanZookeeper utility to accumulo admin command
cshannon opened a new pull request, #2925:
URL: https://github.com/apache/accumulo/pull/2925
Found another utility that can be moved to the accumulo admin command. This utility cleans all old instances out of Zookeeper (except the current one). It's a bit different than the DeleteZooInstance command as that command allows deleting a single instance that is specified so it's worth having both.
--
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 #2925: Adding CleanZookeeper utility to accumulo admin command
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2925:
URL: https://github.com/apache/accumulo/pull/2925#discussion_r969518889
##########
server/base/src/main/java/org/apache/accumulo/server/util/CleanZookeeper.java:
##########
@@ -67,25 +48,26 @@ public static void main(String[] args) {
String instanceNamePath = root + Constants.ZINSTANCES + "/" + instanceName;
byte[] id = zk.getData(instanceNamePath);
if (id != null && !new String(id, UTF_8).equals(context.getInstanceID().canonical())) {
- try {
- zk.recursiveDelete(instanceNamePath, NodeMissingPolicy.SKIP);
- } catch (KeeperException.NoAuthException ex) {
- log.warn("Unable to delete {}", instanceNamePath);
- }
+ delete(zk, instanceNamePath);
+ System.out.println("Deleted instance " + instanceName);
Review Comment:
```suggestion
System.out.println("Deleted instance: " + instanceName);
```
--
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] cshannon commented on pull request #2925: Adding CleanZookeeper utility to accumulo admin command
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #2925:
URL: https://github.com/apache/accumulo/pull/2925#issuecomment-1246968747
@milleruntime - Any thoughts on this before merging since you looked at the others?
--
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] cshannon closed pull request #2925: Adding CleanZookeeper utility to accumulo admin command
Posted by GitBox <gi...@apache.org>.
cshannon closed pull request #2925: Adding CleanZookeeper utility to accumulo admin command
URL: https://github.com/apache/accumulo/pull/2925
--
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] milleruntime commented on pull request #2925: Adding CleanZookeeper utility to accumulo admin command
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2925:
URL: https://github.com/apache/accumulo/pull/2925#issuecomment-1246990161
This looks good but #2914 is a priority and they both touch Admin.
--
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] cshannon commented on a diff in pull request #2925: Adding CleanZookeeper utility to accumulo admin command
Posted by GitBox <gi...@apache.org>.
cshannon commented on code in PR #2925:
URL: https://github.com/apache/accumulo/pull/2925#discussion_r969541610
##########
server/base/src/main/java/org/apache/accumulo/server/util/CleanZookeeper.java:
##########
@@ -67,25 +48,26 @@ public static void main(String[] args) {
String instanceNamePath = root + Constants.ZINSTANCES + "/" + instanceName;
byte[] id = zk.getData(instanceNamePath);
if (id != null && !new String(id, UTF_8).equals(context.getInstanceID().canonical())) {
- try {
- zk.recursiveDelete(instanceNamePath, NodeMissingPolicy.SKIP);
- } catch (KeeperException.NoAuthException ex) {
- log.warn("Unable to delete {}", instanceNamePath);
- }
+ delete(zk, instanceNamePath);
+ System.out.println("Deleted instance " + instanceName);
Review Comment:
Thanks for the review, I should be able to push that quick change tomorrow when I get a chance.
--
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] cshannon commented on pull request #2925: Adding CleanZookeeper utility to accumulo admin command
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #2925:
URL: https://github.com/apache/accumulo/pull/2925#issuecomment-1250056691
Closing this PR out as I'm going to create a new one and just merge the functionality of CleanZookeeper into the DeleteZooInstance utility with a flag as suggested.
--
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 pull request #2925: Adding CleanZookeeper utility to accumulo admin command
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2925:
URL: https://github.com/apache/accumulo/pull/2925#issuecomment-1248362935
> Found another utility that can be moved to the accumulo admin command. This utility cleans all old instances out of Zookeeper (except the current one). It's a bit different than the DeleteZooInstance command as that command allows deleting a single instance that is specified so it's worth having both.
I'm not sure it's worth having both. The delete one could just have a flag to delete all non-current ones.
--
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] cshannon commented on pull request #2925: Adding CleanZookeeper utility to accumulo admin command
Posted by GitBox <gi...@apache.org>.
cshannon commented on PR #2925:
URL: https://github.com/apache/accumulo/pull/2925#issuecomment-1248581056
> > Found another utility that can be moved to the accumulo admin command. This utility cleans all old instances out of Zookeeper (except the current one). It's a bit different than the DeleteZooInstance command as that command allows deleting a single instance that is specified so it's worth having both.
>
> I'm not sure it's worth having both. The delete one could just have a flag to delete all non-current ones.
I like that idea, I can simply move this functionality to the other command pretty easily and have a flag as you suggested. Having 1 utility/command to clean up zookeeper instances is probably less confusing and I can just update the troubleshooting documentation.
If no one else has any other comments or objects I can create a new PR tomorrow (and close this one) that moves the functionality here into the other utility and then removes the CleanZookeeper utility as it won't be needed.
--
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