keith-turner commented on code in PR #4861: URL: https://github.com/apache/accumulo/pull/4861#discussion_r1757723864
########## test/src/main/java/org/apache/accumulo/test/lock/ServiceLockPathsIT.java: ########## @@ -0,0 +1,98 @@ +/* + * 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("CTEST", 3); + cfg.getClusterServerConfiguration().addScanServerResourceGroup("STEST", 2); + cfg.getClusterServerConfiguration().addTabletServerResourceGroup("TTEST", 1); + } + + @Test + public void testPaths() throws Exception { + ServiceLockPaths paths = getServerContext().getServerPaths(); + assertNotNull(paths.getGarbageCollector()); + assertNotNull(paths.getManager()); + assertNull(paths.getMonitor()); // monitor not started + assertEquals(2, paths.getTabletServer(Optional.empty(), Optional.empty()).size()); + assertEquals(1, + paths.getTabletServer(Optional.of(Constants.DEFAULT_RESOURCE_GROUP_NAME), Optional.empty()) + .size()); + assertEquals(1, paths.getTabletServer(Optional.of("TTEST"), Optional.empty()).size()); + assertEquals(0, paths.getTabletServer(Optional.of("FAKE"), Optional.empty()).size()); Review Comment: ```suggestion assertEquals(0, paths.getTabletServer(Optional.of("FAKE"), Optional.empty()).size()); assertEquals(0, paths.getTabletServer(Optional.of("CTEST"), Optional.empty()).size()); assertEquals(0, paths.getTabletServer(Optional.of("STEST"), Optional.empty()).size()); ``` ########## test/src/main/java/org/apache/accumulo/test/lock/ServiceLockPathsIT.java: ########## @@ -0,0 +1,98 @@ +/* + * 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("CTEST", 3); + cfg.getClusterServerConfiguration().addScanServerResourceGroup("STEST", 2); + cfg.getClusterServerConfiguration().addTabletServerResourceGroup("TTEST", 1); + } + + @Test + public void testPaths() throws Exception { + ServiceLockPaths paths = getServerContext().getServerPaths(); + assertNotNull(paths.getGarbageCollector()); + assertNotNull(paths.getManager()); + assertNull(paths.getMonitor()); // monitor not started + assertEquals(2, paths.getTabletServer(Optional.empty(), Optional.empty()).size()); + assertEquals(1, + paths.getTabletServer(Optional.of(Constants.DEFAULT_RESOURCE_GROUP_NAME), Optional.empty()) + .size()); + assertEquals(1, paths.getTabletServer(Optional.of("TTEST"), Optional.empty()).size()); + assertEquals(0, paths.getTabletServer(Optional.of("FAKE"), Optional.empty()).size()); + + assertEquals(4, paths.getCompactor(Optional.empty(), Optional.empty()).size()); + assertEquals(1, paths + .getCompactor(Optional.of(Constants.DEFAULT_RESOURCE_GROUP_NAME), Optional.empty()).size()); + assertEquals(3, paths.getCompactor(Optional.of("CTEST"), Optional.empty()).size()); + assertEquals(0, paths.getCompactor(Optional.of("FAKE"), Optional.empty()).size()); Review Comment: ```suggestion assertEquals(0, paths.getCompactor(Optional.of("FAKE"), Optional.empty()).size()); assertEquals(0, paths.getCompactor(Optional.of("TTEST"), Optional.empty()).size()); assertEquals(0, paths.getCompactor(Optional.of("STEST"), Optional.empty()).size()); ``` ########## core/src/main/java/org/apache/accumulo/core/clientImpl/ZookeeperLockChecker.java: ########## @@ -18,35 +18,55 @@ */ package org.apache.accumulo.core.clientImpl; +import java.util.Optional; +import java.util.Set; + import org.apache.accumulo.core.Constants; import org.apache.accumulo.core.clientImpl.ClientTabletCacheImpl.TabletServerLockChecker; -import org.apache.accumulo.core.fate.zookeeper.ZooCache; import org.apache.accumulo.core.lock.ServiceLock; +import org.apache.accumulo.core.lock.ServiceLockPaths.ServiceLockPath; + +import com.google.common.net.HostAndPort; public class ZookeeperLockChecker implements TabletServerLockChecker { - private final ZooCache zc; + private final ClientContext ctx; private final String root; ZookeeperLockChecker(ClientContext context) { - zc = context.getZooCache(); + this.ctx = context; this.root = context.getZooKeeperRoot() + Constants.ZTSERVERS; } public boolean doesTabletServerLockExist(String server) { - var zLockPath = ServiceLock.path(root + "/" + server); - return ServiceLock.getSessionId(zc, zLockPath) != 0; + // ServiceLockPaths only returns items that have a lock + Set<ServiceLockPath> tservers = ctx.getServerPaths().getTabletServer(Optional.empty(), + Optional.of(HostAndPort.fromString(server))); + if (tservers.isEmpty()) { + return false; + } + return true; } @Override - public boolean isLockHeld(String tserver, String session) { - var zLockPath = ServiceLock.path(root + "/" + tserver); - return ServiceLock.getSessionId(zc, zLockPath) == Long.parseLong(session, 16); + public boolean isLockHeld(String server, String session) { + // ServiceLockPaths only returns items that have a lock + Set<ServiceLockPath> tservers = ctx.getServerPaths().getTabletServer(Optional.empty(), + Optional.of(HostAndPort.fromString(server))); + if (tservers.isEmpty()) { + return false; + } Review Comment: Could drop this to shorten code, will still do same thing. ```suggestion ``` ########## core/src/test/java/org/apache/accumulo/core/lock/ServiceLockPathsTest.java: ########## @@ -0,0 +1,880 @@ +/* + * 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 static org.apache.accumulo.core.Constants.DEFAULT_RESOURCE_GROUP_NAME; +import static org.apache.accumulo.core.Constants.ZCOMPACTORS; +import static org.apache.accumulo.core.Constants.ZDEADTSERVERS; +import static org.apache.accumulo.core.Constants.ZGC_LOCK; +import static org.apache.accumulo.core.Constants.ZMANAGER_LOCK; +import static org.apache.accumulo.core.Constants.ZMINI_LOCK; +import static org.apache.accumulo.core.Constants.ZMONITOR_LOCK; +import static org.apache.accumulo.core.Constants.ZSSERVERS; +import static org.apache.accumulo.core.Constants.ZTABLE_LOCKS; +import static org.apache.accumulo.core.Constants.ZTSERVERS; +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 static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; + +import java.util.Iterator; +import java.util.List; +import java.util.Optional; +import java.util.Set; +import java.util.UUID; + +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 org.apache.accumulo.core.lock.ServiceLockData.ThriftService; +import org.apache.accumulo.core.lock.ServiceLockPaths.ServiceLockPath; +import org.easymock.EasyMock; +import org.junit.jupiter.api.Test; + +import com.google.common.net.HostAndPort; + +public class ServiceLockPathsTest { + + private static final String ROOT = "/accumulo/instance_id"; + private static final String TEST_RESOURCE_GROUP = "TEST_RG"; + private static final String HOSTNAME = "localhost:9876"; + private static final HostAndPort hp = HostAndPort.fromString(HOSTNAME); + + @Test + public void testPathGeneration() { + + ClientContext ctx = EasyMock.createMock(ClientContext.class); + EasyMock.expect(ctx.getZooKeeperRoot()).andReturn(ROOT).anyTimes(); + + EasyMock.replay(ctx); + + ServiceLockPaths paths = new ServiceLockPaths(ctx); + // Test management process path creation + ServiceLockPath slp = paths.createGarbageCollectorPath(); + assertNull(slp.getServer()); + assertNull(slp.getResourceGroup()); + assertEquals(ZGC_LOCK, slp.getType()); + assertEquals(ROOT + ZGC_LOCK, slp.toString()); + + slp = paths.createManagerPath(); + assertNull(slp.getServer()); + assertNull(slp.getResourceGroup()); + assertEquals(ZMANAGER_LOCK, slp.getType()); + assertEquals(ROOT + ZMANAGER_LOCK, slp.toString()); + + assertThrows(NullPointerException.class, () -> paths.createMiniPath(null)); + String miniUUID = UUID.randomUUID().toString(); + slp = paths.createMiniPath(miniUUID); + assertEquals(miniUUID, slp.getServer()); + assertNull(slp.getResourceGroup()); + assertEquals(ZMINI_LOCK, slp.getType()); + assertEquals(ROOT + ZMINI_LOCK + "/" + miniUUID, slp.toString()); + + slp = paths.createMonitorPath(); + assertNull(slp.getServer()); + assertNull(slp.getResourceGroup()); + assertEquals(ZMONITOR_LOCK, slp.getType()); + assertEquals(ROOT + ZMONITOR_LOCK, slp.toString()); + + // Test worker process path creation + assertThrows(NullPointerException.class, () -> paths.createCompactorPath(null, null)); + assertThrows(NullPointerException.class, + () -> paths.createCompactorPath(TEST_RESOURCE_GROUP, null)); + slp = paths.createCompactorPath(TEST_RESOURCE_GROUP, hp); + assertEquals(HOSTNAME, slp.getServer()); + assertEquals(TEST_RESOURCE_GROUP, slp.getResourceGroup()); + assertEquals(ZCOMPACTORS, slp.getType()); + assertEquals(ROOT + ZCOMPACTORS + "/" + TEST_RESOURCE_GROUP + "/" + HOSTNAME, slp.toString()); + + assertThrows(NullPointerException.class, () -> paths.createDeadTabletServerPath(null, null)); + assertThrows(NullPointerException.class, + () -> paths.createDeadTabletServerPath(TEST_RESOURCE_GROUP, null)); + slp = paths.createDeadTabletServerPath(TEST_RESOURCE_GROUP, hp); + assertEquals(HOSTNAME, slp.getServer()); + assertEquals(TEST_RESOURCE_GROUP, slp.getResourceGroup()); + assertEquals(ZDEADTSERVERS, slp.getType()); + assertEquals(ROOT + ZDEADTSERVERS + "/" + TEST_RESOURCE_GROUP + "/" + HOSTNAME, slp.toString()); + + assertThrows(NullPointerException.class, () -> paths.createScanServerPath(null, null)); + assertThrows(NullPointerException.class, + () -> paths.createScanServerPath(TEST_RESOURCE_GROUP, null)); + slp = paths.createScanServerPath(TEST_RESOURCE_GROUP, hp); + assertEquals(HOSTNAME, slp.getServer()); + assertEquals(TEST_RESOURCE_GROUP, slp.getResourceGroup()); + assertEquals(ZSSERVERS, slp.getType()); + assertEquals(ROOT + ZSSERVERS + "/" + TEST_RESOURCE_GROUP + "/" + HOSTNAME, slp.toString()); + + assertThrows(NullPointerException.class, () -> paths.createTabletServerPath(null, null)); + assertThrows(NullPointerException.class, + () -> paths.createTabletServerPath(TEST_RESOURCE_GROUP, null)); + slp = paths.createTabletServerPath(TEST_RESOURCE_GROUP, hp); + assertEquals(HOSTNAME, slp.getServer()); + assertEquals(TEST_RESOURCE_GROUP, slp.getResourceGroup()); + assertEquals(ZTSERVERS, slp.getType()); + assertEquals(ROOT + ZTSERVERS + "/" + TEST_RESOURCE_GROUP + "/" + HOSTNAME, slp.toString()); + + EasyMock.verify(ctx); + } + + @Test + public void testGetGarbageCollectorNotRunning() { + + ClientContext ctx = EasyMock.createMock(ClientContext.class); + ZooCache zc = EasyMock.createMock(ZooCache.class); + + EasyMock.expect(ctx.getZooKeeperRoot()).andReturn(ROOT).anyTimes(); + EasyMock.expect(ctx.getZooCache()).andReturn(zc).anyTimes(); + EasyMock.expect(zc.getChildren(ROOT + ZGC_LOCK)).andReturn(List.of()).anyTimes(); + EasyMock.expect(ctx.getServerPaths()).andReturn(new ServiceLockPaths(ctx)).anyTimes(); + EasyMock.replay(ctx, zc); + + ServiceLockPath slp = ctx.getServerPaths().getGarbageCollector(); + assertNull(slp); + + EasyMock.verify(ctx, zc); + + } + + @Test + public void testGetGarbageCollector() { + + UUID uuid = UUID.randomUUID(); + String svcLock1 = ServiceLock.ZLOCK_PREFIX + uuid.toString() + "#0000000001"; + String svcLock2 = ServiceLock.ZLOCK_PREFIX + uuid.toString() + "#0000000002"; + ServiceLockData sld = + new ServiceLockData(uuid, HOSTNAME, ThriftService.GC, TEST_RESOURCE_GROUP); + + ClientContext ctx = EasyMock.createMock(ClientContext.class); + ZooCache zc = EasyMock.createMock(ZooCache.class); + + EasyMock.expect(ctx.getZooKeeperRoot()).andReturn(ROOT).anyTimes(); + EasyMock.expect(ctx.getZooCache()).andReturn(zc).anyTimes(); + EasyMock.expect(zc.getChildren(ROOT + ZGC_LOCK)).andReturn(List.of(svcLock1, svcLock2)) + .anyTimes(); + EasyMock + .expect(zc.get(EasyMock.eq(ROOT + ZGC_LOCK + "/" + svcLock1), EasyMock.isA(ZcStat.class))) + .andReturn(sld.serialize()); + EasyMock.expect(ctx.getServerPaths()).andReturn(new ServiceLockPaths(ctx)).anyTimes(); + EasyMock.replay(ctx, zc); + + ServiceLockPath slp = ctx.getServerPaths().getGarbageCollector(); + assertNotNull(slp); + assertNull(slp.getServer()); + assertNull(slp.getResourceGroup()); + assertEquals(ZGC_LOCK, slp.getType()); + assertEquals(ROOT + ZGC_LOCK, slp.toString()); + + EasyMock.verify(ctx, zc); + + } + + @Test + public void testGetManagerNotRunning() { + + ClientContext ctx = EasyMock.createMock(ClientContext.class); + ZooCache zc = EasyMock.createMock(ZooCache.class); + + EasyMock.expect(ctx.getZooKeeperRoot()).andReturn(ROOT).anyTimes(); + EasyMock.expect(ctx.getZooCache()).andReturn(zc).anyTimes(); + EasyMock.expect(zc.getChildren(ROOT + ZMANAGER_LOCK)).andReturn(List.of()).anyTimes(); + EasyMock.expect(ctx.getServerPaths()).andReturn(new ServiceLockPaths(ctx)).anyTimes(); + EasyMock.replay(ctx, zc); + + ServiceLockPath slp = ctx.getServerPaths().getManager(); + assertNull(slp); + + EasyMock.verify(ctx, zc); + + } + + @Test + public void testGetManager() { + + UUID uuid = UUID.randomUUID(); + String svcLock1 = ServiceLock.ZLOCK_PREFIX + uuid.toString() + "#0000000001"; + String svcLock2 = ServiceLock.ZLOCK_PREFIX + uuid.toString() + "#0000000002"; + ServiceLockData sld = + new ServiceLockData(uuid, HOSTNAME, ThriftService.MANAGER, TEST_RESOURCE_GROUP); + + ClientContext ctx = EasyMock.createMock(ClientContext.class); + ZooCache zc = EasyMock.createMock(ZooCache.class); + + EasyMock.expect(ctx.getZooKeeperRoot()).andReturn(ROOT).anyTimes(); + EasyMock.expect(ctx.getZooCache()).andReturn(zc).anyTimes(); + EasyMock.expect(zc.getChildren(ROOT + ZMANAGER_LOCK)).andReturn(List.of(svcLock1, svcLock2)) + .anyTimes(); + EasyMock + .expect( + zc.get(EasyMock.eq(ROOT + ZMANAGER_LOCK + "/" + svcLock1), EasyMock.isA(ZcStat.class))) + .andReturn(sld.serialize()); + EasyMock.expect(ctx.getServerPaths()).andReturn(new ServiceLockPaths(ctx)).anyTimes(); + EasyMock.replay(ctx, zc); + + ServiceLockPath slp = ctx.getServerPaths().getManager(); + assertNotNull(slp); + assertNull(slp.getServer()); + assertNull(slp.getResourceGroup()); + assertEquals(ZMANAGER_LOCK, slp.getType()); + assertEquals(ROOT + ZMANAGER_LOCK, slp.toString()); + + EasyMock.verify(ctx, zc); + + } + + @Test + public void testGetMonitorNotRunning() { + + ClientContext ctx = EasyMock.createMock(ClientContext.class); + ZooCache zc = EasyMock.createMock(ZooCache.class); + + EasyMock.expect(ctx.getZooKeeperRoot()).andReturn(ROOT).anyTimes(); + EasyMock.expect(ctx.getZooCache()).andReturn(zc).anyTimes(); + EasyMock.expect(zc.getChildren(ROOT + ZMONITOR_LOCK)).andReturn(List.of()).anyTimes(); + EasyMock.expect(ctx.getServerPaths()).andReturn(new ServiceLockPaths(ctx)).anyTimes(); + EasyMock.replay(ctx, zc); + + ServiceLockPath slp = ctx.getServerPaths().getMonitor(); + assertNull(slp); + + EasyMock.verify(ctx, zc); + + } + + @Test + public void testGetMonitor() { + + UUID uuid = UUID.randomUUID(); + String svcLock1 = ServiceLock.ZLOCK_PREFIX + uuid.toString() + "#0000000001"; + String svcLock2 = ServiceLock.ZLOCK_PREFIX + uuid.toString() + "#0000000002"; + ServiceLockData sld = + new ServiceLockData(uuid, HOSTNAME, ThriftService.NONE, TEST_RESOURCE_GROUP); + + ClientContext ctx = EasyMock.createMock(ClientContext.class); + ZooCache zc = EasyMock.createMock(ZooCache.class); + + EasyMock.expect(ctx.getZooKeeperRoot()).andReturn(ROOT).anyTimes(); + EasyMock.expect(ctx.getZooCache()).andReturn(zc).anyTimes(); + EasyMock.expect(zc.getChildren(ROOT + ZMONITOR_LOCK)).andReturn(List.of(svcLock1, svcLock2)) + .anyTimes(); + EasyMock + .expect( + zc.get(EasyMock.eq(ROOT + ZMONITOR_LOCK + "/" + svcLock1), EasyMock.isA(ZcStat.class))) + .andReturn(sld.serialize()); + EasyMock.expect(ctx.getServerPaths()).andReturn(new ServiceLockPaths(ctx)).anyTimes(); + EasyMock.replay(ctx, zc); + + ServiceLockPath slp = ctx.getServerPaths().getMonitor(); + assertNotNull(slp); + assertNull(slp.getServer()); + assertNull(slp.getResourceGroup()); + assertEquals(ZMONITOR_LOCK, slp.getType()); + assertEquals(ROOT + ZMONITOR_LOCK, slp.toString()); + + EasyMock.verify(ctx, zc); + + } + + @Test + public void testGetCompactorsNotRunning() { + + ClientContext ctx = EasyMock.createMock(ClientContext.class); + ZooCache zc = EasyMock.createMock(ZooCache.class); + + EasyMock.expect(ctx.getZooKeeperRoot()).andReturn(ROOT).anyTimes(); + EasyMock.expect(ctx.getZooCache()).andReturn(zc).anyTimes(); + EasyMock.expect(zc.getChildren(ROOT + ZCOMPACTORS)).andReturn(List.of()).anyTimes(); + EasyMock.expect(ctx.getServerPaths()).andReturn(new ServiceLockPaths(ctx)).anyTimes(); + EasyMock.replay(ctx, zc); + + assertThrows(NullPointerException.class, () -> ctx.getServerPaths().getCompactor(null, null)); + assertThrows(NullPointerException.class, + () -> ctx.getServerPaths().getCompactor(Optional.of(TEST_RESOURCE_GROUP), null)); + assertTrue(ctx.getServerPaths().getCompactor(Optional.empty(), Optional.empty()).isEmpty()); + assertTrue(ctx.getServerPaths().getCompactor(Optional.of(TEST_RESOURCE_GROUP), Optional.empty()) + .isEmpty()); + assertTrue(ctx.getServerPaths().getCompactor(Optional.of(TEST_RESOURCE_GROUP), Optional.of(hp)) + .isEmpty()); + + EasyMock.verify(ctx, zc); + + } + + @Test + public void testGetCompactors() { + + UUID uuid = UUID.randomUUID(); + String svcLock1 = ServiceLock.ZLOCK_PREFIX + uuid.toString() + "#0000000001"; + String svcLock2 = ServiceLock.ZLOCK_PREFIX + uuid.toString() + "#0000000002"; + ServiceLockData sld1 = + new ServiceLockData(uuid, HOSTNAME, ThriftService.COMPACTOR, TEST_RESOURCE_GROUP); + ServiceLockData sld2 = + new ServiceLockData(uuid, HOSTNAME, ThriftService.COMPACTOR, DEFAULT_RESOURCE_GROUP_NAME); + + ClientContext ctx = EasyMock.createMock(ClientContext.class); + ZooCache zc = EasyMock.createMock(ZooCache.class); + + EasyMock.expect(ctx.getZooKeeperRoot()).andReturn(ROOT).anyTimes(); + EasyMock.expect(ctx.getZooCache()).andReturn(zc).anyTimes(); + EasyMock.expect(zc.getChildren(ROOT + ZCOMPACTORS)) + .andReturn(List.of(TEST_RESOURCE_GROUP, DEFAULT_RESOURCE_GROUP_NAME)).anyTimes(); + EasyMock.expect(zc.getChildren(ROOT + ZCOMPACTORS + "/" + TEST_RESOURCE_GROUP)) + .andReturn(List.of(HOSTNAME)).anyTimes(); + EasyMock.expect(zc.getChildren(ROOT + ZCOMPACTORS + "/" + DEFAULT_RESOURCE_GROUP_NAME)) + .andReturn(List.of(HOSTNAME)).anyTimes(); + EasyMock.expect(zc.getChildren(ROOT + ZCOMPACTORS + "/" + TEST_RESOURCE_GROUP + "/" + HOSTNAME)) + .andReturn(List.of(svcLock1, svcLock2)).anyTimes(); + EasyMock + .expect( + zc.getChildren(ROOT + ZCOMPACTORS + "/" + DEFAULT_RESOURCE_GROUP_NAME + "/" + HOSTNAME)) + .andReturn(List.of(svcLock1, svcLock2)).anyTimes(); + EasyMock.expect(zc.get( + EasyMock + .eq(ROOT + ZCOMPACTORS + "/" + TEST_RESOURCE_GROUP + "/" + HOSTNAME + "/" + svcLock1), + EasyMock.isA(ZcStat.class))).andReturn(sld1.serialize()).anyTimes(); + EasyMock + .expect(zc.get(EasyMock.eq(ROOT + ZCOMPACTORS + "/" + DEFAULT_RESOURCE_GROUP_NAME + "/" + + HOSTNAME + "/" + svcLock1), EasyMock.isA(ZcStat.class))) + .andReturn(sld2.serialize()).anyTimes(); + EasyMock.expect(ctx.getServerPaths()).andReturn(new ServiceLockPaths(ctx)).anyTimes(); + EasyMock.replay(ctx, zc); + + // query for all + Set<ServiceLockPath> results = + ctx.getServerPaths().getCompactor(Optional.empty(), Optional.empty()); + assertEquals(2, results.size()); + Iterator<ServiceLockPath> iter = results.iterator(); + ServiceLockPath slp1 = iter.next(); + assertEquals(HOSTNAME, slp1.getServer()); + assertEquals(ZCOMPACTORS, slp1.getType()); + if (slp1.getResourceGroup().equals(TEST_RESOURCE_GROUP)) { + assertEquals(ROOT + ZCOMPACTORS + "/" + TEST_RESOURCE_GROUP + "/" + HOSTNAME, + slp1.toString()); + } else if (slp1.getResourceGroup().equals(DEFAULT_RESOURCE_GROUP_NAME)) { + assertEquals(ROOT + ZCOMPACTORS + "/" + DEFAULT_RESOURCE_GROUP_NAME + "/" + HOSTNAME, + slp1.toString()); + } else { + fail("wrong resource group"); + } + ServiceLockPath slp2 = iter.next(); + assertEquals(HOSTNAME, slp2.getServer()); + assertEquals(ZCOMPACTORS, slp2.getType()); + if (slp2.getResourceGroup().equals(TEST_RESOURCE_GROUP)) { + assertEquals(ROOT + ZCOMPACTORS + "/" + TEST_RESOURCE_GROUP + "/" + HOSTNAME, + slp2.toString()); + } else if (slp2.getResourceGroup().equals(DEFAULT_RESOURCE_GROUP_NAME)) { + assertEquals(ROOT + ZCOMPACTORS + "/" + DEFAULT_RESOURCE_GROUP_NAME + "/" + HOSTNAME, + slp2.toString()); + } else { + fail("wrong resource group"); + } + + // query for all in non-existent resource group + results = + ctx.getServerPaths().getCompactor(Optional.of("FAKE_RESOURCE_GROUP"), Optional.empty()); + assertEquals(0, results.size()); + + // query for all in test resource group + results = ctx.getServerPaths().getCompactor(Optional.of(TEST_RESOURCE_GROUP), Optional.empty()); + assertEquals(1, results.size()); + iter = results.iterator(); + slp1 = iter.next(); + assertEquals(HOSTNAME, slp1.getServer()); + assertEquals(ZCOMPACTORS, slp1.getType()); + assertEquals(TEST_RESOURCE_GROUP, slp1.getResourceGroup()); + assertEquals(ROOT + ZCOMPACTORS + "/" + TEST_RESOURCE_GROUP + "/" + HOSTNAME, slp1.toString()); + + // query for a specific server + results = ctx.getServerPaths().getCompactor(Optional.of(TEST_RESOURCE_GROUP), Optional.of(hp)); + assertEquals(1, results.size()); + iter = results.iterator(); + slp1 = iter.next(); + assertEquals(HOSTNAME, slp1.getServer()); + assertEquals(ZCOMPACTORS, slp1.getType()); + assertEquals(TEST_RESOURCE_GROUP, slp1.getResourceGroup()); + assertEquals(ROOT + ZCOMPACTORS + "/" + TEST_RESOURCE_GROUP + "/" + HOSTNAME, slp1.toString()); + + // query for a wrong server + results = ctx.getServerPaths().getCompactor(Optional.of(TEST_RESOURCE_GROUP), + Optional.of(HostAndPort.fromString("localhost:1234"))); + assertEquals(0, results.size()); + + EasyMock.verify(ctx, zc); + + } + + @Test + public void testGetScanServersNotRunning() { + + ClientContext ctx = EasyMock.createMock(ClientContext.class); + ZooCache zc = EasyMock.createMock(ZooCache.class); + + EasyMock.expect(ctx.getZooKeeperRoot()).andReturn(ROOT).anyTimes(); + EasyMock.expect(ctx.getZooCache()).andReturn(zc).anyTimes(); + EasyMock.expect(zc.getChildren(ROOT + ZSSERVERS)).andReturn(List.of()).anyTimes(); + EasyMock.expect(ctx.getServerPaths()).andReturn(new ServiceLockPaths(ctx)).anyTimes(); + EasyMock.replay(ctx, zc); + + assertThrows(NullPointerException.class, () -> ctx.getServerPaths().getScanServer(null, null)); + assertThrows(NullPointerException.class, + () -> ctx.getServerPaths().getScanServer(Optional.of(TEST_RESOURCE_GROUP), null)); + assertTrue(ctx.getServerPaths().getScanServer(Optional.empty(), Optional.empty()).isEmpty()); + assertTrue(ctx.getServerPaths() + .getScanServer(Optional.of(TEST_RESOURCE_GROUP), Optional.empty()).isEmpty()); + assertTrue(ctx.getServerPaths().getScanServer(Optional.of(TEST_RESOURCE_GROUP), Optional.of(hp)) + .isEmpty()); + + EasyMock.verify(ctx, zc); + + } + + @Test + public void testGetScanServers() { + + UUID uuid = UUID.randomUUID(); + String svcLock1 = ServiceLock.ZLOCK_PREFIX + uuid.toString() + "#0000000001"; + String svcLock2 = ServiceLock.ZLOCK_PREFIX + uuid.toString() + "#0000000002"; + ServiceLockData sld1 = + new ServiceLockData(uuid, HOSTNAME, ThriftService.TABLET_SCAN, TEST_RESOURCE_GROUP); + ServiceLockData sld2 = + new ServiceLockData(uuid, HOSTNAME, ThriftService.TABLET_SCAN, DEFAULT_RESOURCE_GROUP_NAME); + + ClientContext ctx = EasyMock.createMock(ClientContext.class); + ZooCache zc = EasyMock.createMock(ZooCache.class); + + EasyMock.expect(ctx.getZooKeeperRoot()).andReturn(ROOT).anyTimes(); + EasyMock.expect(ctx.getZooCache()).andReturn(zc).anyTimes(); + EasyMock.expect(zc.getChildren(ROOT + ZSSERVERS)) + .andReturn(List.of(TEST_RESOURCE_GROUP, DEFAULT_RESOURCE_GROUP_NAME)).anyTimes(); + EasyMock.expect(zc.getChildren(ROOT + ZSSERVERS + "/" + TEST_RESOURCE_GROUP)) + .andReturn(List.of(HOSTNAME)).anyTimes(); + EasyMock.expect(zc.getChildren(ROOT + ZSSERVERS + "/" + DEFAULT_RESOURCE_GROUP_NAME)) Review Comment: Would it be worthwhile to test more than one sever in a RG and zero servers in an RG in ZK? ########## server/base/src/main/java/org/apache/accumulo/server/manager/LiveTServerSet.java: ########## @@ -228,15 +230,13 @@ public synchronized void scanServers() { final Set<TServerInstance> updates = new HashSet<>(); final Set<TServerInstance> doomed = new HashSet<>(); - final String path = context.getZooKeeperRoot() + Constants.ZTSERVERS; + Set<ServiceLockPath> tservers = + context.getServerPaths().getTabletServer(Optional.empty(), Optional.empty()); Review Comment: Will this only return tservers that have a lock entry under them? If so then that may impact the functionality in this class that cleans up tserver w/ no lock. ########## test/src/main/java/org/apache/accumulo/test/lock/ServiceLockPathsIT.java: ########## @@ -0,0 +1,98 @@ +/* + * 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("CTEST", 3); + cfg.getClusterServerConfiguration().addScanServerResourceGroup("STEST", 2); + cfg.getClusterServerConfiguration().addTabletServerResourceGroup("TTEST", 1); + } + + @Test + public void testPaths() throws Exception { + ServiceLockPaths paths = getServerContext().getServerPaths(); + assertNotNull(paths.getGarbageCollector()); + assertNotNull(paths.getManager()); + assertNull(paths.getMonitor()); // monitor not started + assertEquals(2, paths.getTabletServer(Optional.empty(), Optional.empty()).size()); + assertEquals(1, + paths.getTabletServer(Optional.of(Constants.DEFAULT_RESOURCE_GROUP_NAME), Optional.empty()) + .size()); + assertEquals(1, paths.getTabletServer(Optional.of("TTEST"), Optional.empty()).size()); + assertEquals(0, paths.getTabletServer(Optional.of("FAKE"), Optional.empty()).size()); + + assertEquals(4, paths.getCompactor(Optional.empty(), Optional.empty()).size()); + assertEquals(1, paths + .getCompactor(Optional.of(Constants.DEFAULT_RESOURCE_GROUP_NAME), Optional.empty()).size()); + assertEquals(3, paths.getCompactor(Optional.of("CTEST"), Optional.empty()).size()); + assertEquals(0, paths.getCompactor(Optional.of("FAKE"), Optional.empty()).size()); + + assertEquals(3, paths.getScanServer(Optional.empty(), Optional.empty()).size()); + assertEquals(1, + paths.getScanServer(Optional.of(Constants.DEFAULT_RESOURCE_GROUP_NAME), Optional.empty()) + .size()); + assertEquals(2, paths.getScanServer(Optional.of("STEST"), Optional.empty()).size()); + assertEquals(0, paths.getScanServer(Optional.of("FAKE"), Optional.empty()).size()); Review Comment: ```suggestion assertEquals(0, paths.getScanServer(Optional.of("FAKE"), Optional.empty()).size()); assertEquals(0, paths.getScanServer(Optional.of("CTEST"), Optional.empty()).size()); assertEquals(0, paths.getScanServer(Optional.of("TTEST"), Optional.empty()).size()); ``` ########## test/src/main/java/org/apache/accumulo/test/functional/TabletResourceGroupBalanceIT.java: ########## @@ -104,21 +97,9 @@ public static Map<String,String> getTServerGroups(MiniAccumuloClusterImpl cluste throws Exception { Map<String,String> tservers = new HashMap<>(); - ZooCache zk = cluster.getServerContext().getZooCache(); - String zpath = cluster.getServerContext().getZooKeeperRoot() + Constants.ZTSERVERS; - - List<String> children = zk.getChildren(zpath); - for (String child : children) { - final var zLockPath = ServiceLock.path(zpath + "/" + child); - ZcStat stat = new ZcStat(); - Optional<ServiceLockData> sld = ServiceLock.getLockData(zk, zLockPath, stat); - try { - HostAndPort client = sld.orElseThrow().getAddress(ServiceLockData.ThriftService.TSERV); - String resourceGroup = sld.orElseThrow().getGroup(ServiceLockData.ThriftService.TSERV); - tservers.put(client.toString(), resourceGroup); - } catch (NoSuchElementException nsee) { - // We are starting and stopping servers, so it's possible for this to occur. - } + for (ServiceLockPath tserver : cluster.getServerContext().getServerPaths() Review Comment: This loop is so much better than the code it replaces. ########## core/src/main/java/org/apache/accumulo/core/clientImpl/ZookeeperLockChecker.java: ########## @@ -18,35 +18,55 @@ */ package org.apache.accumulo.core.clientImpl; +import java.util.Optional; +import java.util.Set; + import org.apache.accumulo.core.Constants; import org.apache.accumulo.core.clientImpl.ClientTabletCacheImpl.TabletServerLockChecker; -import org.apache.accumulo.core.fate.zookeeper.ZooCache; import org.apache.accumulo.core.lock.ServiceLock; +import org.apache.accumulo.core.lock.ServiceLockPaths.ServiceLockPath; + +import com.google.common.net.HostAndPort; public class ZookeeperLockChecker implements TabletServerLockChecker { - private final ZooCache zc; + private final ClientContext ctx; private final String root; ZookeeperLockChecker(ClientContext context) { - zc = context.getZooCache(); + this.ctx = context; this.root = context.getZooKeeperRoot() + Constants.ZTSERVERS; } public boolean doesTabletServerLockExist(String server) { - var zLockPath = ServiceLock.path(root + "/" + server); - return ServiceLock.getSessionId(zc, zLockPath) != 0; + // ServiceLockPaths only returns items that have a lock + Set<ServiceLockPath> tservers = ctx.getServerPaths().getTabletServer(Optional.empty(), + Optional.of(HostAndPort.fromString(server))); + if (tservers.isEmpty()) { + return false; + } + return true; Review Comment: ```suggestion return tsevers.isPresent(); ``` ########## core/src/main/java/org/apache/accumulo/core/clientImpl/InstanceOperationsImpl.java: ########## @@ -234,20 +235,14 @@ public Set<String> getScanServers() { @Override public List<String> getTabletServers() { - ZooCache cache = context.getZooCache(); - String path = context.getZooKeeperRoot() + Constants.ZTSERVERS; + Set<ServiceLockPath> paths = + context.getServerPaths().getTabletServer(Optional.empty(), Optional.empty()); List<String> results = new ArrayList<>(); - for (String candidate : cache.getChildren(path)) { - var children = cache.getChildren(path + "/" + candidate); - if (children != null && !children.isEmpty()) { - var copy = new ArrayList<>(children); - Collections.sort(copy); - var data = cache.get(path + "/" + candidate + "/" + copy.get(0)); - if (data != null && !"manager".equals(new String(data, UTF_8))) { - results.add(candidate); - } + paths.forEach(p -> { + if (!p.getServer().equals("manager")) { Review Comment: What is going on with this check for `manager`? Is this still needed? ########## core/src/main/java/org/apache/accumulo/core/lock/ServiceLockPaths.java: ########## @@ -0,0 +1,385 @@ +/* + * 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; + } + + } + + private final ClientContext ctx; + + public ServiceLockPaths(ClientContext context) { + this.ctx = context; + } + + private static String determineServerType(final String path) { + if (path.contains(Constants.ZGC_LOCK)) { + return Constants.ZGC_LOCK; + } else if (path.contains(Constants.ZMANAGER_LOCK)) { + return Constants.ZMANAGER_LOCK; + } else if (path.contains(Constants.ZMONITOR_LOCK)) { + return Constants.ZMONITOR_LOCK; + } else if (path.contains(Constants.ZMINI_LOCK)) { + return Constants.ZMINI_LOCK; + } else if (path.contains(Constants.ZCOMPACTORS)) { + return Constants.ZCOMPACTORS; + } else if (path.contains(Constants.ZSSERVERS)) { + return Constants.ZSSERVERS; + } else if (path.contains(Constants.ZDEADTSERVERS)) { + // This has to be before TSERVERS + return Constants.ZDEADTSERVERS; + } else if (path.contains(Constants.ZTSERVERS)) { + return Constants.ZTSERVERS; + } else { + throw new IllegalArgumentException("Unhandled to determine server type from path: " + path); + } + } + + /** + * Parse a ZooKeeper path string and return a ServiceLockPath + */ + public static ServiceLockPath parse(Optional<String> serverType, String path) { + Objects.requireNonNull(serverType); + Objects.requireNonNull(path); + + final String type = serverType.isEmpty() ? determineServerType(path) : serverType.orElseThrow(); Review Comment: ```suggestion final String type = serverType.orElseGet(()->determineServerType(path)); ``` -- 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]
