keith-turner commented on code in PR #4861: URL: https://github.com/apache/accumulo/pull/4861#discussion_r1748581816
########## core/src/main/java/org/apache/accumulo/core/lock/ServiceLockPaths.java: ########## @@ -0,0 +1,367 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.accumulo.core.lock; + +import java.util.HashSet; +import java.util.List; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; + +import org.apache.accumulo.core.Constants; +import org.apache.accumulo.core.clientImpl.ClientContext; +import org.apache.accumulo.core.fate.zookeeper.ZooCache; +import org.apache.accumulo.core.fate.zookeeper.ZooCache.ZcStat; + +import com.google.common.base.Preconditions; +import com.google.common.net.HostAndPort; + +/** + * Class for creating and retrieving ServiceLockPath objects + */ +public class ServiceLockPaths { + + public static class ServiceLockPath { + private final String type; + private final String resourceGroup; + private final String server; + private final String path; + + /** + * Exists for ServiceLockIt + */ + protected ServiceLockPath(String path) { + this.type = null; + this.resourceGroup = null; + this.server = null; + this.path = path; + } + + /** + * Create a ServiceLockPath for a management process + */ + private ServiceLockPath(String root, String type) { + Objects.requireNonNull(root); + this.type = Objects.requireNonNull(type); + Preconditions.checkArgument(this.type.equals(Constants.ZGC_LOCK) Review Comment: This type feels like it could be an enum instead of a string. If it were an enum then could create a private static enum set and check its existence here. ########## test/src/main/java/org/apache/accumulo/test/lock/ServiceLockPathsIT.java: ########## @@ -0,0 +1,105 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.accumulo.test.lock; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; + +import java.util.Optional; + +import org.apache.accumulo.core.Constants; +import org.apache.accumulo.core.conf.Property; +import org.apache.accumulo.core.lock.ServiceLockPaths; +import org.apache.accumulo.harness.AccumuloClusterHarness; +import org.apache.accumulo.minicluster.ServerType; +import org.apache.accumulo.miniclusterImpl.MiniAccumuloConfigImpl; +import org.apache.accumulo.test.util.Wait; +import org.apache.hadoop.conf.Configuration; +import org.junit.jupiter.api.Test; + +public class ServiceLockPathsIT extends AccumuloClusterHarness { + + @Override + public void configureMiniCluster(MiniAccumuloConfigImpl cfg, Configuration hadoopCoreSite) { + cfg.setProperty(Property.INSTANCE_ZK_TIMEOUT, "10s"); + cfg.getClusterServerConfiguration().setNumDefaultCompactors(1); + cfg.getClusterServerConfiguration().setNumDefaultScanServers(1); + cfg.getClusterServerConfiguration().setNumDefaultTabletServers(1); + cfg.getClusterServerConfiguration().addCompactorResourceGroup("TEST", 1); + cfg.getClusterServerConfiguration().addScanServerResourceGroup("TEST", 1); + cfg.getClusterServerConfiguration().addTabletServerResourceGroup("TEST", 1); Review Comment: Could add some more unique RG names and numbers. Then can check that CTEST RG only shows up from compactors and scan or tablet servers. Adjsuting the numbers is nice for testing this functionality, but could cause memory problems because too many processes are starting so not sure about that. Somehow it would be nice to test more than one of the server type. ```suggestion cfg.getClusterServerConfiguration().addCompactorResourceGroup("TEST", 1); cfg.getClusterServerConfiguration().addCompactorResourceGroup("CTEST", 2); cfg.getClusterServerConfiguration().addScanServerResourceGroup("TEST", 1); cfg.getClusterServerConfiguration().addScanServerResourceGroup("STEST", 3); cfg.getClusterServerConfiguration().addTabletServerResourceGroup("TEST", 1); cfg.getClusterServerConfiguration().addTabletServerResourceGroup("TTEST", 4); ``` ########## core/src/main/java/org/apache/accumulo/core/lock/ServiceLockPaths.java: ########## @@ -0,0 +1,367 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.accumulo.core.lock; + +import java.util.HashSet; +import java.util.List; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; + +import org.apache.accumulo.core.Constants; +import org.apache.accumulo.core.clientImpl.ClientContext; +import org.apache.accumulo.core.fate.zookeeper.ZooCache; +import org.apache.accumulo.core.fate.zookeeper.ZooCache.ZcStat; + +import com.google.common.base.Preconditions; +import com.google.common.net.HostAndPort; + +/** + * Class for creating and retrieving ServiceLockPath objects + */ +public class ServiceLockPaths { Review Comment: This is really nice pulling all of this code into one place. Seems like we may be able to get to a point where the only use of the constants like ZTSERVERS is inside this class. Looking around in the code I found one constant that was used outside of this class ZCOMPACTORS which is used in ExternalCompactionUtil. Could the code in ExternalCompactionUtil be refactored to use this class? ########## core/src/main/java/org/apache/accumulo/core/lock/ServiceLockPaths.java: ########## @@ -0,0 +1,367 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.accumulo.core.lock; + +import java.util.HashSet; +import java.util.List; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; + +import org.apache.accumulo.core.Constants; +import org.apache.accumulo.core.clientImpl.ClientContext; +import org.apache.accumulo.core.fate.zookeeper.ZooCache; +import org.apache.accumulo.core.fate.zookeeper.ZooCache.ZcStat; + +import com.google.common.base.Preconditions; +import com.google.common.net.HostAndPort; + +/** + * Class for creating and retrieving ServiceLockPath objects + */ +public class ServiceLockPaths { + + public static class ServiceLockPath { + private final String type; + private final String resourceGroup; + private final String server; + private final String path; + + /** + * Exists for ServiceLockIt + */ + protected ServiceLockPath(String path) { + this.type = null; + this.resourceGroup = null; + this.server = null; + this.path = path; + } + + /** + * Create a ServiceLockPath for a management process + */ + private ServiceLockPath(String root, String type) { + Objects.requireNonNull(root); + this.type = Objects.requireNonNull(type); + Preconditions.checkArgument(this.type.equals(Constants.ZGC_LOCK) + || this.type.equals(Constants.ZMANAGER_LOCK) || this.type.equals(Constants.ZMONITOR_LOCK) + || this.type.equals(Constants.ZTABLE_LOCKS), "Unsupported type: " + type); + // These server types support only one active instance, so they use a lock at + // a known path, not the server's address. + this.resourceGroup = null; + this.server = null; + this.path = root + this.type; + } + + /** + * Create a ServiceLockPath for ZTABLE_LOCKS + */ + private ServiceLockPath(String root, String type, String content) { + Objects.requireNonNull(root); + this.type = Objects.requireNonNull(type); + Preconditions.checkArgument( + this.type.equals(Constants.ZTABLE_LOCKS) || this.type.equals(Constants.ZMINI_LOCK), + "Unsupported type: " + type); + this.resourceGroup = null; + this.server = Objects.requireNonNull(content); + this.path = root + this.type + "/" + this.server; + } + + /** + * Create a ServiceLockPath for a worker process + */ + private ServiceLockPath(String root, String type, String resourceGroup, String server) { + Objects.requireNonNull(root); + this.type = Objects.requireNonNull(type); + Preconditions.checkArgument( + this.type.equals(Constants.ZCOMPACTORS) || this.type.equals(Constants.ZSSERVERS) + || this.type.equals(Constants.ZTSERVERS) || this.type.equals(Constants.ZDEADTSERVERS), Review Comment: I am not sure what this Constants.ZDEADTSERVERS is. Curious if you know what it is? I am going to look into it later. ########## core/src/main/java/org/apache/accumulo/core/lock/ServiceLockPaths.java: ########## @@ -0,0 +1,367 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.accumulo.core.lock; + +import java.util.HashSet; +import java.util.List; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; + +import org.apache.accumulo.core.Constants; +import org.apache.accumulo.core.clientImpl.ClientContext; +import org.apache.accumulo.core.fate.zookeeper.ZooCache; +import org.apache.accumulo.core.fate.zookeeper.ZooCache.ZcStat; + +import com.google.common.base.Preconditions; +import com.google.common.net.HostAndPort; + +/** + * Class for creating and retrieving ServiceLockPath objects + */ +public class ServiceLockPaths { + + public static class ServiceLockPath { + private final String type; + private final String resourceGroup; + private final String server; + private final String path; + + /** + * Exists for ServiceLockIt + */ + protected ServiceLockPath(String path) { + this.type = null; + this.resourceGroup = null; + this.server = null; + this.path = path; + } + + /** + * Create a ServiceLockPath for a management process + */ + private ServiceLockPath(String root, String type) { + Objects.requireNonNull(root); + this.type = Objects.requireNonNull(type); + Preconditions.checkArgument(this.type.equals(Constants.ZGC_LOCK) + || this.type.equals(Constants.ZMANAGER_LOCK) || this.type.equals(Constants.ZMONITOR_LOCK) + || this.type.equals(Constants.ZTABLE_LOCKS), "Unsupported type: " + type); + // These server types support only one active instance, so they use a lock at + // a known path, not the server's address. + this.resourceGroup = null; + this.server = null; + this.path = root + this.type; + } + + /** + * Create a ServiceLockPath for ZTABLE_LOCKS + */ + private ServiceLockPath(String root, String type, String content) { + Objects.requireNonNull(root); + this.type = Objects.requireNonNull(type); + Preconditions.checkArgument( + this.type.equals(Constants.ZTABLE_LOCKS) || this.type.equals(Constants.ZMINI_LOCK), + "Unsupported type: " + type); + this.resourceGroup = null; + this.server = Objects.requireNonNull(content); + this.path = root + this.type + "/" + this.server; + } + + /** + * Create a ServiceLockPath for a worker process + */ + private ServiceLockPath(String root, String type, String resourceGroup, String server) { + Objects.requireNonNull(root); + this.type = Objects.requireNonNull(type); + Preconditions.checkArgument( + this.type.equals(Constants.ZCOMPACTORS) || this.type.equals(Constants.ZSSERVERS) + || this.type.equals(Constants.ZTSERVERS) || this.type.equals(Constants.ZDEADTSERVERS), + "Unsupported type: " + type); + this.resourceGroup = Objects.requireNonNull(resourceGroup); + this.server = Objects.requireNonNull(server); + this.path = root + this.type + "/" + this.resourceGroup + "/" + this.server; + } + + public String getType() { + return type; + } + + public String getResourceGroup() { + return resourceGroup; + } + + public String getServer() { + return server; + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + if (obj == null) { + return false; + } + if (getClass() != obj.getClass()) { + return false; + } + ServiceLockPath other = (ServiceLockPath) obj; + if (path == null) { + if (other.path != null) { + return false; + } + } else if (!path.equals(other.path)) { + return false; + } + if (resourceGroup == null) { + if (other.resourceGroup != null) { + return false; + } + } else if (!resourceGroup.equals(other.resourceGroup)) { + return false; + } + if (server == null) { + if (other.server != null) { + return false; + } + } else if (!server.equals(other.server)) { + return false; + } + if (type == null) { + if (other.type != null) { + return false; + } + } else if (!type.equals(other.type)) { + return false; + } + return true; + } + + @Override + public int hashCode() { + final int prime = 31; + int result = 1; + result = prime * result + ((path == null) ? 0 : path.hashCode()); + result = prime * result + ((resourceGroup == null) ? 0 : resourceGroup.hashCode()); + result = prime * result + ((server == null) ? 0 : server.hashCode()); + result = prime * result + ((type == null) ? 0 : type.hashCode()); + return result; + } + + @Override + public String toString() { + return this.path; + } + + } + + /** + * Parse a ZooKeeper path string and return a ServiceLockPath + */ + public static ServiceLockPath parse(String path) { + Objects.requireNonNull(path); + if (path.contains(Constants.ZGC_LOCK)) { + return new ServiceLockPath(path.substring(0, path.indexOf(Constants.ZGC_LOCK)), + Constants.ZGC_LOCK); + } else if (path.contains(Constants.ZMANAGER_LOCK)) { + return new ServiceLockPath(path.substring(0, path.indexOf(Constants.ZMANAGER_LOCK)), + Constants.ZMANAGER_LOCK); + } else if (path.contains(Constants.ZMONITOR_LOCK)) { + return new ServiceLockPath(path.substring(0, path.indexOf(Constants.ZMONITOR_LOCK)), Review Comment: The three else ifs have to scan the entire string. Looking at how this method is used it currently used in two places and those two places compose a string from parts, so the parts are known in those instances. May be possible to optimize this overall, not sure. Like it seems like the callers know the server type, so that could maybe passed in. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
