[GitHub] [accumulo] ctubbsii commented on a change in pull request #1907: Rename Master to Manager in class names.

[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