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 2021/02/04 13:32:55 UTC
[GitHub] [accumulo] brianloss opened a new pull request #1907: Rename Master to Manager in class names.
brianloss opened a new pull request #1907:
URL: https://github.com/apache/accumulo/pull/1907
This change renames any non-deprecated classes having "master" in the
class name to the same name but with "master" replaced by "manager."
* Use IDE refactoring to rename classes having "Master" in the name.
* Update accumulo-cluster script to handle the renaming of the
entrypoint Master to Manager.
* Update master.thrift to include class renaming but also rename the
thrift file itself from master.thrift to manager.thrift.
re #1642
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1907: Rename Master to Manager in class names.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1907:
URL: https://github.com/apache/accumulo/pull/1907#issuecomment-774054234
> @ctubbsii it seems removing the Manager main method broke minicluster, so I pushed a fix for that in a new commit. I tried to make use of the KeywordExecutable class there too.
Ah, I didn't think about minicluster using them. I figured it was using our Main class entry point. Thanks for catching that.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] brianloss commented on a change in pull request #1907: Rename Master to Manager in class names.
Posted by GitBox <gi...@apache.org>.
brianloss commented on a change in pull request #1907:
URL: https://github.com/apache/accumulo/pull/1907#discussion_r570313939
##########
File path: test/src/main/java/org/apache/accumulo/test/start/KeywordStartIT.java
##########
@@ -162,7 +163,7 @@ public void checkHasMain() {
expectSet.add(Info.class);
expectSet.add(Initialize.class);
expectSet.add(LoginProperties.class);
- expectSet.add(Master.class);
+ expectSet.add(Manager.class);
Review comment:
Got it now. I believe this is resolved with the commit I just pushed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1907: Rename Master to Manager in class names.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1907:
URL: https://github.com/apache/accumulo/pull/1907#discussion_r570304567
##########
File path: core/src/main/thrift/manager.thrift
##########
@@ -213,7 +213,7 @@ service FateService {
}
-service MasterClientService extends FateService {
Review comment:
Wire compatibility is definitely broken between 2.0 and 2.1 for server-to-server communication (preventing rolling upgrades, for example), but I don't know that we have any explicit check to determine client-to-server wire compatibility. So, it may or may not be broken. I'm not sure.
I'm okay with breaking it... if we document that in the release notes. I just want to ensure we are noticing the consequences and accepting the risks as we go.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] brianloss commented on a change in pull request #1907: Rename Master to Manager in class names.
Posted by GitBox <gi...@apache.org>.
brianloss commented on a change in pull request #1907:
URL: https://github.com/apache/accumulo/pull/1907#discussion_r570267039
##########
File path: test/src/main/java/org/apache/accumulo/test/start/KeywordStartIT.java
##########
@@ -162,7 +163,7 @@ public void checkHasMain() {
expectSet.add(Info.class);
expectSet.add(Initialize.class);
expectSet.add(LoginProperties.class);
- expectSet.add(Master.class);
+ expectSet.add(Manager.class);
Review comment:
ManagerExecutable and MasterExecutable (deprecated) both exist, but their execute methods both call Manager.main. But you're saying I should add an empty deprecated Master (in the old package) that extends Manager but has a main method in case a user invoked directly instead of via the accumulo scripts?
##########
File path: core/src/main/thrift/manager.thrift
##########
@@ -213,7 +213,7 @@ service FateService {
}
-service MasterClientService extends FateService {
Review comment:
Ah, interesting. I thought you said in a previous PR that wire compatibility was already broken between 2.0 and 2.1. If it's not, then yeah, we might want to reconsider.
##########
File path: test/src/main/java/org/apache/accumulo/test/start/KeywordStartIT.java
##########
@@ -162,7 +163,7 @@ public void checkHasMain() {
expectSet.add(Info.class);
expectSet.add(Initialize.class);
expectSet.add(LoginProperties.class);
- expectSet.add(Master.class);
+ expectSet.add(Manager.class);
Review comment:
ManagerExecutable and MasterExecutable (deprecated) both exist, but their execute methods both call Manager.main. Are you saying I should add an empty deprecated Master (in the old package) that extends Manager but has a main method in case a user invoked directly instead of via the accumulo scripts?
##########
File path: test/src/main/java/org/apache/accumulo/test/start/KeywordStartIT.java
##########
@@ -162,7 +163,7 @@ public void checkHasMain() {
expectSet.add(Info.class);
expectSet.add(Initialize.class);
expectSet.add(LoginProperties.class);
- expectSet.add(Master.class);
+ expectSet.add(Manager.class);
Review comment:
Got it now. I believe this is resolved with the commit I just pushed.
##########
File path: core/src/main/thrift/manager.thrift
##########
@@ -213,7 +213,7 @@ service FateService {
}
-service MasterClientService extends FateService {
Review comment:
Hmm, I'm not sure. I think if it's not already broken, we're better off not doing any changes to the thrift package in this release (since we already chose to hold the package renaming to support balancer compatibility). I'll do a sniff test with the 2.0.1 client against a 2.1 (main branch) snapshot and see if I can uncover any issues.
##########
File path: core/src/main/thrift/manager.thrift
##########
@@ -213,7 +213,7 @@ service FateService {
}
-service MasterClientService extends FateService {
Review comment:
I spun up a cluster from a tarball built from the main branch, and then ran the accumulo shell from a 2.0.1 install (but client properties point to my 2.1 snapshot instance), and while some things worked in the shell, scanning a table hung indefinitely (but worked from the 2.1 shell), so I'd say client compatibility was already broken. Given that, it would seem the best approach is just to document that fact in the release notes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] brianloss commented on a change in pull request #1907: Rename Master to Manager in class names.
Posted by GitBox <gi...@apache.org>.
brianloss commented on a change in pull request #1907:
URL: https://github.com/apache/accumulo/pull/1907#discussion_r570316895
##########
File path: core/src/main/thrift/manager.thrift
##########
@@ -213,7 +213,7 @@ service FateService {
}
-service MasterClientService extends FateService {
Review comment:
Hmm, I'm not sure. I think if it's not already broken, we're better off not doing any changes to the thrift package in this release (since we already chose to hold the package renaming to support balancer compatibility). I'll do a sniff test with the 2.0.1 client against a 2.1 (main branch) snapshot and see if I can uncover any issues.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] brianloss merged pull request #1907: Rename Master to Manager in class names.
Posted by GitBox <gi...@apache.org>.
brianloss merged pull request #1907:
URL: https://github.com/apache/accumulo/pull/1907
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] brianloss commented on a change in pull request #1907: Rename Master to Manager in class names.
Posted by GitBox <gi...@apache.org>.
brianloss commented on a change in pull request #1907:
URL: https://github.com/apache/accumulo/pull/1907#discussion_r570268080
##########
File path: core/src/main/thrift/manager.thrift
##########
@@ -213,7 +213,7 @@ service FateService {
}
-service MasterClientService extends FateService {
Review comment:
Ah, interesting. I thought you said in a previous PR that wire compatibility was already broken between 2.0 and 2.1. If it's not, then yeah, we might want to reconsider.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] brianloss commented on a change in pull request #1907: Rename Master to Manager in class names.
Posted by GitBox <gi...@apache.org>.
brianloss commented on a change in pull request #1907:
URL: https://github.com/apache/accumulo/pull/1907#discussion_r570267039
##########
File path: test/src/main/java/org/apache/accumulo/test/start/KeywordStartIT.java
##########
@@ -162,7 +163,7 @@ public void checkHasMain() {
expectSet.add(Info.class);
expectSet.add(Initialize.class);
expectSet.add(LoginProperties.class);
- expectSet.add(Master.class);
+ expectSet.add(Manager.class);
Review comment:
ManagerExecutable and MasterExecutable (deprecated) both exist, but their execute methods both call Manager.main. Are you saying I should add an empty deprecated Master (in the old package) that extends Manager but has a main method in case a user invoked directly instead of via the accumulo scripts?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1907: Rename Master to Manager in class names.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1907:
URL: https://github.com/apache/accumulo/pull/1907#discussion_r570301535
##########
File path: test/src/main/java/org/apache/accumulo/test/start/KeywordStartIT.java
##########
@@ -162,7 +163,7 @@ public void checkHasMain() {
expectSet.add(Info.class);
expectSet.add(Initialize.class);
expectSet.add(LoginProperties.class);
- expectSet.add(Master.class);
+ expectSet.add(Manager.class);
Review comment:
> Are you saying I should add an empty deprecated Master (in the old package) that extends Manager but has a main method in case a user invoked directly instead of via the accumulo scripts?
Yes. That is why the test existed. When I originally added the KeywordExecutable stuff, Eric had advocated for ensuring support for executing server processes directly with their own main methods (via `java path.to.actual.main.Class` or via `bin/accumulo path.to.actual.main.Class`), because that had previously worked, and some users may still be executing them that way.
However, this would *only* apply to the old class names that previously existed with a main method, for backwards compatibility. Newer class names would not already be in use by users, so they don't need to be introduced with a main method, because there's no backwards compatibility argument to have one.
In the end, this argument for backwards compatibility is pretty obscure. It is most likely that users are executing these with the scripts (which use our `start.Main keyword` entry points). So, if we decide to remove the main methods, that's fine... but it wouldn't make sense to rename the class in this test, since the only point of this test is to check backwards compatibility of the original class name having a main method.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] brianloss commented on a change in pull request #1907: Rename Master to Manager in class names.
Posted by GitBox <gi...@apache.org>.
brianloss commented on a change in pull request #1907:
URL: https://github.com/apache/accumulo/pull/1907#discussion_r570441340
##########
File path: core/src/main/thrift/manager.thrift
##########
@@ -213,7 +213,7 @@ service FateService {
}
-service MasterClientService extends FateService {
Review comment:
I spun up a cluster from a tarball built from the main branch, and then ran the accumulo shell from a 2.0.1 install (but client properties point to my 2.1 snapshot instance), and while some things worked in the shell, scanning a table hung indefinitely (but worked from the 2.1 shell), so I'd say client compatibility was already broken. Given that, it would seem the best approach is just to document that fact in the release notes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1907: Rename Master to Manager in class names.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1907:
URL: https://github.com/apache/accumulo/pull/1907#discussion_r570250903
##########
File path: test/src/main/java/org/apache/accumulo/test/start/KeywordStartIT.java
##########
@@ -162,7 +163,7 @@ public void checkHasMain() {
expectSet.add(Info.class);
expectSet.add(Initialize.class);
expectSet.add(LoginProperties.class);
- expectSet.add(Master.class);
+ expectSet.add(Manager.class);
Review comment:
This should remain pointed to `org.apache.accumulo.master.Master`, because it's checking that a `main` method isn't deleted, potentially breaking user scripts. However, because we launch the `Manager` using `KeywordExecutable`, it is not necessary for the new `org.apache.accumulo.manager.Manager` to have a `main` method.
##########
File path: core/src/main/thrift/manager.thrift
##########
@@ -213,7 +213,7 @@ service FateService {
}
-service MasterClientService extends FateService {
Review comment:
Will renaming this will break client-server compatibility between 2.0 and 2.1, if it is not already broken?
I'm okay if we draw a line in the sand and say that 2.0 clients can't talk to 2.1, but I thought that there was some previous decisions made to try to preserve that to some extent, and I can't recall the details.
##########
File path: test/src/main/java/org/apache/accumulo/test/start/KeywordStartIT.java
##########
@@ -162,7 +163,7 @@ public void checkHasMain() {
expectSet.add(Info.class);
expectSet.add(Initialize.class);
expectSet.add(LoginProperties.class);
- expectSet.add(Master.class);
+ expectSet.add(Manager.class);
Review comment:
Yes. That is why the test existed. When I originally added the KeywordExecutable stuff, Eric had advocated for ensuring support for executing server processes directly with their own main methods (via `java path.to.actual.main.Class` or via `bin/accumulo path.to.actual.main.Class`), because that had previously worked, and some users may still be executing them that way.
However, this would *only* apply to the old class names that previously existed with a main method, for backwards compatibility. Newer class names would not already be in use by users, so they don't need to be introduced with a main method, because there's no backwards compatibility argument to have one.
In the end, this argument for backwards compatibility is pretty obscure. It is most likely that users are executing these with the scripts (which use our `start.Main keyword` entry points). So, if we decide to remove the main methods, that's fine... but it wouldn't make sense to rename the class in this test, since the only point of this test is to check backwards compatibility of the original class name having a main method.
##########
File path: test/src/main/java/org/apache/accumulo/test/start/KeywordStartIT.java
##########
@@ -162,7 +163,7 @@ public void checkHasMain() {
expectSet.add(Info.class);
expectSet.add(Initialize.class);
expectSet.add(LoginProperties.class);
- expectSet.add(Master.class);
+ expectSet.add(Manager.class);
Review comment:
> Are you saying I should add an empty deprecated Master (in the old package) that extends Manager but has a main method in case a user invoked directly instead of via the accumulo scripts?
Yes. That is why the test existed. When I originally added the KeywordExecutable stuff, Eric had advocated for ensuring support for executing server processes directly with their own main methods (via `java path.to.actual.main.Class` or via `bin/accumulo path.to.actual.main.Class`), because that had previously worked, and some users may still be executing them that way.
However, this would *only* apply to the old class names that previously existed with a main method, for backwards compatibility. Newer class names would not already be in use by users, so they don't need to be introduced with a main method, because there's no backwards compatibility argument to have one.
In the end, this argument for backwards compatibility is pretty obscure. It is most likely that users are executing these with the scripts (which use our `start.Main keyword` entry points). So, if we decide to remove the main methods, that's fine... but it wouldn't make sense to rename the class in this test, since the only point of this test is to check backwards compatibility of the original class name having a main method.
##########
File path: core/src/main/thrift/manager.thrift
##########
@@ -213,7 +213,7 @@ service FateService {
}
-service MasterClientService extends FateService {
Review comment:
Wire compatibility is definitely broken between 2.0 and 2.1 for server-to-server communication (preventing rolling upgrades, for example), but I don't know that we have any explicit check to determine client-to-server wire compatibility. So, it may or may not be broken. I'm not sure.
I'm okay with breaking it... if we document that in the release notes. I just want to ensure we are noticing the consequences and accepting the risks as we go.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] brianloss commented on pull request #1907: Rename Master to Manager in class names.
Posted by GitBox <gi...@apache.org>.
brianloss commented on pull request #1907:
URL: https://github.com/apache/accumulo/pull/1907#issuecomment-773605391
@ctubbsii it seems removing the Manager main method broke minicluster, so I pushed a fix for that in a new commit. I tried to make use of the KeywordExecutable class there 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1907: Rename Master to Manager in class names.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1907:
URL: https://github.com/apache/accumulo/pull/1907#discussion_r570250903
##########
File path: test/src/main/java/org/apache/accumulo/test/start/KeywordStartIT.java
##########
@@ -162,7 +163,7 @@ public void checkHasMain() {
expectSet.add(Info.class);
expectSet.add(Initialize.class);
expectSet.add(LoginProperties.class);
- expectSet.add(Master.class);
+ expectSet.add(Manager.class);
Review comment:
This should remain pointed to `org.apache.accumulo.master.Master`, because it's checking that a `main` method isn't deleted, potentially breaking user scripts. However, because we launch the `Manager` using `KeywordExecutable`, it is not necessary for the new `org.apache.accumulo.manager.Manager` to have a `main` method.
##########
File path: core/src/main/thrift/manager.thrift
##########
@@ -213,7 +213,7 @@ service FateService {
}
-service MasterClientService extends FateService {
Review comment:
Will renaming this will break client-server compatibility between 2.0 and 2.1, if it is not already broken?
I'm okay if we draw a line in the sand and say that 2.0 clients can't talk to 2.1, but I thought that there was some previous decisions made to try to preserve that to some extent, and I can't recall the details.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] brianloss commented on a change in pull request #1907: Rename Master to Manager in class names.
Posted by GitBox <gi...@apache.org>.
brianloss commented on a change in pull request #1907:
URL: https://github.com/apache/accumulo/pull/1907#discussion_r570267039
##########
File path: test/src/main/java/org/apache/accumulo/test/start/KeywordStartIT.java
##########
@@ -162,7 +163,7 @@ public void checkHasMain() {
expectSet.add(Info.class);
expectSet.add(Initialize.class);
expectSet.add(LoginProperties.class);
- expectSet.add(Master.class);
+ expectSet.add(Manager.class);
Review comment:
ManagerExecutable and MasterExecutable (deprecated) both exist, but their execute methods both call Manager.main. But you're saying I should add an empty deprecated Master (in the old package) that extends Manager but has a main method in case a user invoked directly instead of via the accumulo scripts?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] brianloss commented on pull request #1907: Rename Master to Manager in class names.
Posted by GitBox <gi...@apache.org>.
brianloss commented on pull request #1907:
URL: https://github.com/apache/accumulo/pull/1907#issuecomment-773605391
@ctubbsii it seems removing the Manager main method broke minicluster, so I pushed a fix for that in a new commit. I tried to make use of the KeywordExecutable class there 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1907: Rename Master to Manager in class names.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1907:
URL: https://github.com/apache/accumulo/pull/1907#discussion_r570301535
##########
File path: test/src/main/java/org/apache/accumulo/test/start/KeywordStartIT.java
##########
@@ -162,7 +163,7 @@ public void checkHasMain() {
expectSet.add(Info.class);
expectSet.add(Initialize.class);
expectSet.add(LoginProperties.class);
- expectSet.add(Master.class);
+ expectSet.add(Manager.class);
Review comment:
Yes. That is why the test existed. When I originally added the KeywordExecutable stuff, Eric had advocated for ensuring support for executing server processes directly with their own main methods (via `java path.to.actual.main.Class` or via `bin/accumulo path.to.actual.main.Class`), because that had previously worked, and some users may still be executing them that way.
However, this would *only* apply to the old class names that previously existed with a main method, for backwards compatibility. Newer class names would not already be in use by users, so they don't need to be introduced with a main method, because there's no backwards compatibility argument to have one.
In the end, this argument for backwards compatibility is pretty obscure. It is most likely that users are executing these with the scripts (which use our `start.Main keyword` entry points). So, if we decide to remove the main methods, that's fine... but it wouldn't make sense to rename the class in this test, since the only point of this test is to check backwards compatibility of the original class name having a main method.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org