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:
[email protected]
