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