keith-turner commented on code in PR #4891:
URL: https://github.com/apache/accumulo/pull/4891#discussion_r1761831226
##########
test/src/main/java/org/apache/accumulo/test/lock/ServiceLockPathsIT.java:
##########
@@ -50,54 +50,55 @@ public void configureMiniCluster(MiniAccumuloConfigImpl
cfg, Configuration hadoo
@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(0, paths.getTabletServer(Optional.of("CTEST"),
Optional.empty()).size());
- assertEquals(0, paths.getTabletServer(Optional.of("STEST"),
Optional.empty()).size());
-
- assertEquals(4, paths.getCompactor(Optional.empty(),
Optional.empty()).size());
+ assertNotNull(paths.getGarbageCollector(true));
+ assertNotNull(paths.getManager(true));
+ assertNull(paths.getMonitor(true)); // monitor not started
+ assertEquals(2, paths.getTabletServer(Optional.empty(), Optional.empty(),
true).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(0, paths.getCompactor(Optional.of("TTEST"),
Optional.empty()).size());
- assertEquals(0, paths.getCompactor(Optional.of("STEST"),
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());
- assertEquals(0, paths.getScanServer(Optional.of("CTEST"),
Optional.empty()).size());
- assertEquals(0, paths.getScanServer(Optional.of("TTEST"),
Optional.empty()).size());
+ .getTabletServer(Optional.of(Constants.DEFAULT_RESOURCE_GROUP_NAME),
Optional.empty(), true)
+ .size());
+ assertEquals(1, paths.getTabletServer(Optional.of("TTEST"),
Optional.empty(), true).size());
+ assertEquals(0, paths.getTabletServer(Optional.of("FAKE"),
Optional.empty(), true).size());
+ assertEquals(0, paths.getTabletServer(Optional.of("CTEST"),
Optional.empty(), true).size());
+ assertEquals(0, paths.getTabletServer(Optional.of("STEST"),
Optional.empty(), true).size());
+
+ assertEquals(4, paths.getCompactor(Optional.empty(), Optional.empty(),
true).size());
+ assertEquals(1, paths
+ .getCompactor(Optional.of(Constants.DEFAULT_RESOURCE_GROUP_NAME),
Optional.empty(), true)
+ .size());
+ assertEquals(3, paths.getCompactor(Optional.of("CTEST"), Optional.empty(),
true).size());
+ assertEquals(0, paths.getCompactor(Optional.of("FAKE"), Optional.empty(),
true).size());
+ assertEquals(0, paths.getCompactor(Optional.of("TTEST"), Optional.empty(),
true).size());
+ assertEquals(0, paths.getCompactor(Optional.of("STEST"), Optional.empty(),
true).size());
+
+ assertEquals(3, paths.getScanServer(Optional.empty(), Optional.empty(),
true).size());
+ assertEquals(1, paths
+ .getScanServer(Optional.of(Constants.DEFAULT_RESOURCE_GROUP_NAME),
Optional.empty(), true)
+ .size());
+ assertEquals(2, paths.getScanServer(Optional.of("STEST"),
Optional.empty(), true).size());
+ assertEquals(0, paths.getScanServer(Optional.of("FAKE"), Optional.empty(),
true).size());
+ assertEquals(0, paths.getScanServer(Optional.of("CTEST"),
Optional.empty(), true).size());
+ assertEquals(0, paths.getScanServer(Optional.of("TTEST"),
Optional.empty(), true).size());
getCluster().getClusterControl().stopAllServers(ServerType.COMPACTOR);
- Wait.waitFor(() -> paths.getCompactor(Optional.empty(),
Optional.empty()).size() == 0);
+ Wait.waitFor(() -> paths.getCompactor(Optional.empty(), Optional.empty(),
true).size() == 0);
getCluster().getClusterControl().stopAllServers(ServerType.SCAN_SERVER);
- Wait.waitFor(() -> paths.getScanServer(Optional.empty(),
Optional.empty()).size() == 0);
+ Wait.waitFor(() -> paths.getScanServer(Optional.empty(), Optional.empty(),
true).size() == 0);
getCluster().getClusterControl().stopAllServers(ServerType.GARBAGE_COLLECTOR);
- Wait.waitFor(() -> paths.getGarbageCollector() == null);
+ Wait.waitFor(() -> paths.getGarbageCollector(true) == null);
getCluster().getClusterControl().stopAllServers(ServerType.MANAGER);
- Wait.waitFor(() -> paths.getManager() == null);
+ Wait.waitFor(() -> paths.getManager(true) == null);
getCluster().getClusterControl().stopAllServers(ServerType.TABLET_SERVER);
- Wait.waitFor(() -> paths.getTabletServer(Optional.empty(),
Optional.empty()).size() == 0);
+ Wait.waitFor(() -> paths.getTabletServer(Optional.empty(),
Optional.empty(), true).size() == 0);
Review Comment:
Would be good to test the case of not having locks. Wonder if the following
would work for that. The manager cleans up empty nodes, but its killed before
the tservers are so the the empty nodes should still be there.
```suggestion
Wait.waitFor(() -> paths.getTabletServer(Optional.empty(),
Optional.empty(), true).size() == 0);
assertEquals(2, paths.getTabletServer(Optional.empty(),
Optional.empty(), false).size());
```
##########
core/src/main/java/org/apache/accumulo/core/lock/ServiceLockPaths.java:
##########
@@ -362,15 +377,16 @@ private Set<ServiceLockPath> get(final String serverType,
Optional<String> resou
final ZcStat stat = new ZcStat();
final ServiceLockPath slp =
parse(Optional.of(serverType), typePath + "/" + group + "/" +
server);
- if (slp.getType().equals(Constants.ZDEADTSERVERS)
- && (address.isEmpty() ||
address.orElseThrow().toString().equals(server))) {
- // Dead TServers don't have lock data
- results.add(slp);
- } else {
- Optional<ServiceLockData> sld = ServiceLock.getLockData(cache,
slp, stat);
- if (!sld.isEmpty()
- && (address.isEmpty() ||
address.orElseThrow().toString().equals(server))) {
+ if (address.isEmpty() ||
address.orElseThrow().toString().equals(server)) {
+ if (!withLock || slp.getType().equals(Constants.ZDEADTSERVERS)) {
+ // Dead TServers don't have lock data
results.add(slp);
+ } else {
+ Optional<ServiceLockData> sld = ServiceLock.getLockData(cache,
slp, stat);
+ if (!sld.isEmpty()
+ && (address.isEmpty() ||
address.orElseThrow().toString().equals(server))) {
Review Comment:
Seems like the address already being checked by the outer if stmt and could
be removed here.
```suggestion
if (!sld.isEmpty()) {
```
--
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]
