Skip to content

Modified MiniAccumuloCluster configuration to include cluster config#3533

Merged
dlmarion merged 3 commits into
apache:elasticityfrom
dlmarion:mac-cluster-configuration
Jun 23, 2023
Merged

Modified MiniAccumuloCluster configuration to include cluster config#3533
dlmarion merged 3 commits into
apache:elasticityfrom
dlmarion:mac-cluster-configuration

Conversation

@dlmarion
Copy link
Copy Markdown
Contributor

Created an object used by MiniAccumuloConfigImpl that contains the
number of Compactions, ScanServers, and TabletServers for the default
resource group. This object allows test writers to change the defaults
and add other resource groups for their test. When MAC starts, it starts
all of the Compactors, ScanServers, and TabletServers for all of the groups.
Resource groups can be added to the configuration in the middle of a test
and then calling start(ServerType) will start up the new processes.

@dlmarion dlmarion requested a review from keith-turner June 23, 2023 16:29
@dlmarion
Copy link
Copy Markdown
Contributor Author

I ran a bunch of the modified ITs and a lot of them succeeded. The ones that failed were failing due to something still not working in the elasticity branch.

*
* @param numTservers the number of tablet servers that mini accumulo cluster should start
*/
// ELASTICITY_TODO: Deprecate in 3.0.0 and remove in elasticity on the merge
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will need a method to set the server config in the public API. Also when deprecating something its nice to introduce the replacement if its feasible, so wondering if this should be done in main.

Suggested change
// ELASTICITY_TODO: Deprecate in 3.0.0 and remove in elasticity on the merge
/**
* @since 3.1
*/
public MiniAccumuloConfig setServerConfiguration(ClusterServerConfiguration serverConfig){
impl.setServerConfiguration(serverConfig);
return this;
}
// ELASTICITY_TODO: Deprecate in 3.0.0 and remove in elasticity on the merge


import org.apache.accumulo.core.lock.ServiceLockData;

public class ClusterServerConfiguration {
Copy link
Copy Markdown
Contributor

@keith-turner keith-turner Jun 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class will need to move to the public API or the public API will need something similar.

Also thinking it would be nice to make the class immutable, this could be done with a builder.

class ClusterServerConfiguration {
   static class Builder {
          private Map<String,Integer> compactors = COMPACTORS_DEFAULT;
          private Map<String,Integer> sservers = SSERVERS_DEFAULT;
          private Map<String,Integer> tservers = TSERVERS_DEFAULT;

          Builder setScanServers(Map<String,Integer> scanServerGroups, boolean appendDefautls){
            if(!appendDefaults){
               sservers = Map.copyOf(scanServerGroups);
             } else {
               var copy = new HashMap(scanServerGroups);
               copy.putAll(SSERVERS_DEFAULT);
               // make it immutable
               sservers = Map.copyOf(copy);
             }
             return this;
          }

          Builder setTabletServers(Map<String,Integer> tabletServerGroups, boolean appendDefautls){
             //TODO
          }

           Builder setCompactors(Map<String,Integer> compactorGroups, boolean appendDefautls){
            // TODO
          }

         ClusterServerConfiguration build(){
            // this could be a private constructor
            return new ClusterServerConfiguration(tservers, sservers, compactors);
        }
   }

  static Builder builder()  { return new Builder();}
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can move to public API, but it's not immutable. In the tests we are starting and stopping services. Currently you modify the configuration and call start (if you want more compactors for example). I don't want to swap out the config for a brand new one at runtime.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking when a new config is set it reconciles that with what is currently running and starts/stops processes. Not sure what that would look like in practice.

We could also have an internal non public API version that is mutable. I think making the API version immutable is nice as minis public API does not allow starting and stopping servers after the cluster starts, that seems to be something internal that we use for testing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take a look at ExternalCompation1_IT in this PR for how I'm using it currently.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll create a follow-on to evaluate putting ClusterServerConfiguration in the public API, I want to get this merged so I can fold it into #3496 and keep moving forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

Create a cluster configuration object to use when starting MAC

3 participants