You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "ctubbsii (via GitHub)" <gi...@apache.org> on 2023/04/28 08:17:20 UTC
[GitHub] [accumulo] ctubbsii opened a new pull request, #3361: Remove remaining master references
ctubbsii opened a new pull request, #3361:
URL: https://github.com/apache/accumulo/pull/3361
* Relocate Thrift classes in master.thrift to manager.thrift that are still in use
* Remove unused RecoveryException thrift class
* Update references to LICENSE files in git repos to local bundled copies
* Remove transition checks for slaves/masters files and services in our assembly scripts and configs
* Update generated property docs to clarify that `master.*` were the former names and are not still currently available
* Update SimpleLoadBalancer javadoc to remove reference to deleted file (residual from #3117)
* Stop updating ZK in SetGoalState, as that should have already been done when upgrading to 2.1, and close ServerContext with try-with-resources in that class
* Remove unused RenameMasterDirInZK class (called by SetGoalState, but no more)
* Update javadoc for KeywordExecutable to point to correct main branch in AutoService repo
This fixes #2487
--
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] EdColeman commented on a diff in pull request #3361: Remove remaining master references
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3361:
URL: https://github.com/apache/accumulo/pull/3361#discussion_r1180487788
##########
server/manager/src/main/java/org/apache/accumulo/manager/state/SetGoalState.java:
##########
@@ -45,12 +44,12 @@ public static void main(String[] args) throws Exception {
try {
var siteConfig = SiteConfiguration.auto();
SecurityUtil.serverLogin(siteConfig);
- var context = new ServerContext(siteConfig);
- RenameMasterDirInZK.renameMasterDirInZK(context);
- context.waitForZookeeperAndHdfs();
- context.getZooReaderWriter().putPersistentData(
- context.getZooKeeperRoot() + Constants.ZMANAGER_GOAL_STATE, args[0].getBytes(UTF_8),
- NodeExistsPolicy.OVERWRITE);
+ try (var context = new ServerContext(siteConfig)) {
Review Comment:
Its not a big issue - just something that I thought may help. But if you you feel otherwise, that's fine too.
--
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] EdColeman commented on a diff in pull request #3361: Remove remaining master references
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3361:
URL: https://github.com/apache/accumulo/pull/3361#discussion_r1180471555
##########
server/manager/src/main/java/org/apache/accumulo/manager/state/SetGoalState.java:
##########
@@ -45,12 +44,12 @@ public static void main(String[] args) throws Exception {
try {
var siteConfig = SiteConfiguration.auto();
SecurityUtil.serverLogin(siteConfig);
- var context = new ServerContext(siteConfig);
- RenameMasterDirInZK.renameMasterDirInZK(context);
- context.waitForZookeeperAndHdfs();
- context.getZooReaderWriter().putPersistentData(
- context.getZooKeeperRoot() + Constants.ZMANAGER_GOAL_STATE, args[0].getBytes(UTF_8),
- NodeExistsPolicy.OVERWRITE);
+ try (var context = new ServerContext(siteConfig)) {
Review Comment:
Is it safe to have `close()` called on ServerContext here? The try-with-resources will be a slight change in behavior. Really just wanted to note it here - not an issue as long as the change is expected.
--
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 #3361: Remove remaining master references
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #3361:
URL: https://github.com/apache/accumulo/pull/3361#issuecomment-1527669292
> When I was looking at that code it gave me pause and a comment would have clarified that it was safe / expected without needing to understand why it was safe in that context.
I suspect it gave you pause because you know extra things about how context is passed around widely in other threads inside our server classes. But that's clearly not the case 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
[GitHub] [accumulo] ctubbsii merged pull request #3361: Remove remaining master references
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii merged PR #3361:
URL: https://github.com/apache/accumulo/pull/3361
--
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] EdColeman commented on a diff in pull request #3361: Remove remaining master references
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3361:
URL: https://github.com/apache/accumulo/pull/3361#discussion_r1180477107
##########
server/manager/src/main/java/org/apache/accumulo/manager/state/SetGoalState.java:
##########
@@ -45,12 +44,12 @@ public static void main(String[] args) throws Exception {
try {
var siteConfig = SiteConfiguration.auto();
SecurityUtil.serverLogin(siteConfig);
- var context = new ServerContext(siteConfig);
- RenameMasterDirInZK.renameMasterDirInZK(context);
- context.waitForZookeeperAndHdfs();
- context.getZooReaderWriter().putPersistentData(
- context.getZooKeeperRoot() + Constants.ZMANAGER_GOAL_STATE, args[0].getBytes(UTF_8),
- NodeExistsPolicy.OVERWRITE);
+ try (var context = new ServerContext(siteConfig)) {
Review Comment:
Maybe add a comment? Mainly to call-out that close is being called and safe in this context.
--
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 a diff in pull request #3361: Remove remaining master references
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3361:
URL: https://github.com/apache/accumulo/pull/3361#discussion_r1180493307
##########
server/manager/src/main/java/org/apache/accumulo/manager/state/SetGoalState.java:
##########
@@ -45,12 +44,12 @@ public static void main(String[] args) throws Exception {
try {
var siteConfig = SiteConfiguration.auto();
SecurityUtil.serverLogin(siteConfig);
- var context = new ServerContext(siteConfig);
- RenameMasterDirInZK.renameMasterDirInZK(context);
- context.waitForZookeeperAndHdfs();
- context.getZooReaderWriter().putPersistentData(
- context.getZooKeeperRoot() + Constants.ZMANAGER_GOAL_STATE, args[0].getBytes(UTF_8),
- NodeExistsPolicy.OVERWRITE);
+ try (var context = new ServerContext(siteConfig)) {
Review Comment:
The entire contents of the file could fit on a small Post-It note (if you omit the import statements and license header). I don't think it will take anybody any substantial amount of time to realize that context is not being used anywhere else. And there's no special or unique situation to call out. So, I'd struggle to even know what I could say that would be helpful in a comment.
--
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 a diff in pull request #3361: Remove remaining master references
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3361:
URL: https://github.com/apache/accumulo/pull/3361#discussion_r1180475629
##########
server/manager/src/main/java/org/apache/accumulo/manager/state/SetGoalState.java:
##########
@@ -45,12 +44,12 @@ public static void main(String[] args) throws Exception {
try {
var siteConfig = SiteConfiguration.auto();
SecurityUtil.serverLogin(siteConfig);
- var context = new ServerContext(siteConfig);
- RenameMasterDirInZK.renameMasterDirInZK(context);
- context.waitForZookeeperAndHdfs();
- context.getZooReaderWriter().putPersistentData(
- context.getZooKeeperRoot() + Constants.ZMANAGER_GOAL_STATE, args[0].getBytes(UTF_8),
- NodeExistsPolicy.OVERWRITE);
+ try (var context = new ServerContext(siteConfig)) {
Review Comment:
Yes, it's safe in this `main` method for `SetGoalState`. We're exiting immediately after anyway. The only thing my change did was make sure the resource was closed so it shut down any internal thread pools before the process exits. This got rid of a warning after removing the other line. I'm not sure why my IDE didn't see the warning before, but it didn't. It only noticed the unclosed resource after I deleted the other line.
--
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] EdColeman commented on a diff in pull request #3361: Remove remaining master references
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3361:
URL: https://github.com/apache/accumulo/pull/3361#discussion_r1180471555
##########
server/manager/src/main/java/org/apache/accumulo/manager/state/SetGoalState.java:
##########
@@ -45,12 +44,12 @@ public static void main(String[] args) throws Exception {
try {
var siteConfig = SiteConfiguration.auto();
SecurityUtil.serverLogin(siteConfig);
- var context = new ServerContext(siteConfig);
- RenameMasterDirInZK.renameMasterDirInZK(context);
- context.waitForZookeeperAndHdfs();
- context.getZooReaderWriter().putPersistentData(
- context.getZooKeeperRoot() + Constants.ZMANAGER_GOAL_STATE, args[0].getBytes(UTF_8),
- NodeExistsPolicy.OVERWRITE);
+ try (var context = new ServerContext(siteConfig)) {
Review Comment:
Is it safe to have `close()` called on ServerContext here? The try-with-resources will be a slight change in behavior.
--
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 a diff in pull request #3361: Remove remaining master references
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3361:
URL: https://github.com/apache/accumulo/pull/3361#discussion_r1180481489
##########
server/manager/src/main/java/org/apache/accumulo/manager/state/SetGoalState.java:
##########
@@ -45,12 +44,12 @@ public static void main(String[] args) throws Exception {
try {
var siteConfig = SiteConfiguration.auto();
SecurityUtil.serverLogin(siteConfig);
- var context = new ServerContext(siteConfig);
- RenameMasterDirInZK.renameMasterDirInZK(context);
- context.waitForZookeeperAndHdfs();
- context.getZooReaderWriter().putPersistentData(
- context.getZooKeeperRoot() + Constants.ZMANAGER_GOAL_STATE, args[0].getBytes(UTF_8),
- NodeExistsPolicy.OVERWRITE);
+ try (var context = new ServerContext(siteConfig)) {
Review Comment:
Closing a closable resource is the normal case. There's nothing to comment on. Being unable to close it would be unusual... but in the cases where we can't close it... it's because it's still being used. Also not unusual.
I'm not sure why somebody would think that this *wasn't* safe.
--
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] EdColeman commented on a diff in pull request #3361: Remove remaining master references
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3361:
URL: https://github.com/apache/accumulo/pull/3361#discussion_r1180486199
##########
server/manager/src/main/java/org/apache/accumulo/manager/state/SetGoalState.java:
##########
@@ -45,12 +44,12 @@ public static void main(String[] args) throws Exception {
try {
var siteConfig = SiteConfiguration.auto();
SecurityUtil.serverLogin(siteConfig);
- var context = new ServerContext(siteConfig);
- RenameMasterDirInZK.renameMasterDirInZK(context);
- context.waitForZookeeperAndHdfs();
- context.getZooReaderWriter().putPersistentData(
- context.getZooKeeperRoot() + Constants.ZMANAGER_GOAL_STATE, args[0].getBytes(UTF_8),
- NodeExistsPolicy.OVERWRITE);
+ try (var context = new ServerContext(siteConfig)) {
Review Comment:
Basically - calling close on the server context in the manager seem that it could be unintentional / unsafe if the context was being used elsewhere - it would take someone looking at the code additional time to figure that out and a simple comment might save someone same time.
--
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