You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "EdColeman (via GitHub)" <gi...@apache.org> on 2023/09/25 17:59:48 UTC
[GitHub] [accumulo] EdColeman opened a new pull request, #3776: Count dead tservers to continue in ManagerRepairsDualAssignmentIT
EdColeman opened a new pull request, #3776:
URL: https://github.com/apache/accumulo/pull/3776
Adds a check that requires an increase in the dead server count after a tserver is killed before proceeding with the test, This eliminates the closed connection issue seen with reading the `TabletLocationState` as reported in #3762
This is an alternate implementation for #3771. Only one of these should be merged,
The question in #3771 about strengthening the exception handling in `TabletLocationState` implementation is still open. it is unclear if the code should to try an handle the EOF exception or to just continue throwing the runtime exception.
This fixes #3762 if implemented.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3776: Count dead tservers to continue in ManagerRepairsDualAssignmentIT
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3776:
URL: https://github.com/apache/accumulo/pull/3776#discussion_r1336296449
##########
test/src/main/java/org/apache/accumulo/test/ManagerRepairsDualAssignmentIT.java:
##########
@@ -99,12 +102,21 @@ public void test() throws Exception {
}
}
assertEquals(2, states.size());
+ int deadCount = getZkDeadCount(cluster.getServerContext().getZooReader(),
+ cluster.getServerContext().getInstanceID());
Review Comment:
It should - but I wanted to avoid the assumption in case there was any chance of a concurrent test had / left something there - a very remote possibility. It just seemed more general to get the count. kill a server and wait for that count to increase.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3776: Count dead tservers to continue in ManagerRepairsDualAssignmentIT
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3776:
URL: https://github.com/apache/accumulo/pull/3776#discussion_r1336310583
##########
test/src/main/java/org/apache/accumulo/test/ManagerRepairsDualAssignmentIT.java:
##########
@@ -99,12 +102,21 @@ public void test() throws Exception {
}
}
assertEquals(2, states.size());
+ int deadCount = getZkDeadCount(cluster.getServerContext().getZooReader(),
+ cluster.getServerContext().getInstanceID());
// Kill a tablet server... we don't care which one... wait for everything to be reassigned
cluster.killProcess(ServerType.TABLET_SERVER,
cluster.getProcesses().get(ServerType.TABLET_SERVER).iterator().next());
Set<TabletMetadata.Location> replStates = new HashSet<>();
@SuppressWarnings("deprecation")
TableId repTable = org.apache.accumulo.core.replication.ReplicationTable.ID;
+
+ while (getZkDeadCount(cluster.getServerContext().getZooReader(),
+ cluster.getServerContext().getInstanceID()) <= deadCount) {
Review Comment:
Within the context of the test it should not decrease - whatever would clean the dead server list would only have a narrow window (5 sec). If it is shown to be a problem, then the check would need to be more complicated.
If somehow the "same" server was brought back to life, the remainder of the test might have issue too.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3776: Count dead tservers to continue in ManagerRepairsDualAssignmentIT
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3776:
URL: https://github.com/apache/accumulo/pull/3776#discussion_r1339706116
##########
test/src/main/java/org/apache/accumulo/test/ManagerRepairsDualAssignmentIT.java:
##########
@@ -99,12 +102,21 @@ public void test() throws Exception {
}
}
assertEquals(2, states.size());
+ int deadCount = getZkDeadCount(cluster.getServerContext().getZooReader(),
+ cluster.getServerContext().getInstanceID());
Review Comment:
If a concurrent test was somehow using this test's instance of MiniAccumuloCluster, that'd be a big problem, likely to be the cause of other test flakes. So, we'd want to detect that, not obscure it by progressing with a different count that was detected. I would just add a simple `assertEquals(0, actual)` here instead of trying to record and track the previous number.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #3776: Count dead tservers to continue in ManagerRepairsDualAssignmentIT
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #3776:
URL: https://github.com/apache/accumulo/pull/3776#issuecomment-1740185509
Closing in favor of #3789
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii closed pull request #3776: Count dead tservers to continue in ManagerRepairsDualAssignmentIT
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii closed pull request #3776: Count dead tservers to continue in ManagerRepairsDualAssignmentIT
URL: https://github.com/apache/accumulo/pull/3776
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3776: Count dead tservers to continue in ManagerRepairsDualAssignmentIT
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3776:
URL: https://github.com/apache/accumulo/pull/3776#discussion_r1336310583
##########
test/src/main/java/org/apache/accumulo/test/ManagerRepairsDualAssignmentIT.java:
##########
@@ -99,12 +102,21 @@ public void test() throws Exception {
}
}
assertEquals(2, states.size());
+ int deadCount = getZkDeadCount(cluster.getServerContext().getZooReader(),
+ cluster.getServerContext().getInstanceID());
// Kill a tablet server... we don't care which one... wait for everything to be reassigned
cluster.killProcess(ServerType.TABLET_SERVER,
cluster.getProcesses().get(ServerType.TABLET_SERVER).iterator().next());
Set<TabletMetadata.Location> replStates = new HashSet<>();
@SuppressWarnings("deprecation")
TableId repTable = org.apache.accumulo.core.replication.ReplicationTable.ID;
+
+ while (getZkDeadCount(cluster.getServerContext().getZooReader(),
+ cluster.getServerContext().getInstanceID()) <= deadCount) {
Review Comment:
Within the context of the test it should not decrease - whatever would clean the dead server list would only have a narrow window (5 sec). If it is shown to be a problem, then the check would need to be more complicated.
If somehow the "same" server was brought back to like, the remainder of the test might have issue too.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3776: Count dead tservers to continue in ManagerRepairsDualAssignmentIT
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3776:
URL: https://github.com/apache/accumulo/pull/3776#discussion_r1336324904
##########
test/src/main/java/org/apache/accumulo/test/ManagerRepairsDualAssignmentIT.java:
##########
@@ -150,12 +164,24 @@ public void test() throws Exception {
}
}
+ private int getZkDeadCount(ZooReader zooReader, final InstanceId iid) {
+ String tPath = Constants.ZROOT + "/" + iid + Constants.ZDEADTSERVERS;
+ try {
+ int count = zooReader.getChildren(tPath).size();
+ log.debug("Current dead server count: {}", count);
+ return count;
+ } catch (Exception ex) {
+ throw new IllegalStateException(
+ "Failed to read the number of dead tservers reported in ZooKeeper", ex);
+ }
Review Comment:
Changed in 4994f630bf
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3776: Count dead tservers to continue in ManagerRepairsDualAssignmentIT
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3776:
URL: https://github.com/apache/accumulo/pull/3776#discussion_r1336290592
##########
test/src/main/java/org/apache/accumulo/test/ManagerRepairsDualAssignmentIT.java:
##########
@@ -99,12 +102,21 @@ public void test() throws Exception {
}
}
assertEquals(2, states.size());
+ int deadCount = getZkDeadCount(cluster.getServerContext().getZooReader(),
+ cluster.getServerContext().getInstanceID());
Review Comment:
This should always be 0 at the start of the test.
##########
test/src/main/java/org/apache/accumulo/test/ManagerRepairsDualAssignmentIT.java:
##########
@@ -150,12 +164,24 @@ public void test() throws Exception {
}
}
+ private int getZkDeadCount(ZooReader zooReader, final InstanceId iid) {
+ String tPath = Constants.ZROOT + "/" + iid + Constants.ZDEADTSERVERS;
+ try {
+ int count = zooReader.getChildren(tPath).size();
+ log.debug("Current dead server count: {}", count);
+ return count;
+ } catch (Exception ex) {
+ throw new IllegalStateException(
+ "Failed to read the number of dead tservers reported in ZooKeeper", ex);
+ }
Review Comment:
You don't need to catch and rethrow the exception in the test code... just let it be thrown out of the test method and JUnit will ensure we see the stack trace. It doesn't need to be converted to a RTE.
##########
test/src/main/java/org/apache/accumulo/test/ManagerRepairsDualAssignmentIT.java:
##########
@@ -99,12 +102,21 @@ public void test() throws Exception {
}
}
assertEquals(2, states.size());
+ int deadCount = getZkDeadCount(cluster.getServerContext().getZooReader(),
+ cluster.getServerContext().getInstanceID());
// Kill a tablet server... we don't care which one... wait for everything to be reassigned
cluster.killProcess(ServerType.TABLET_SERVER,
cluster.getProcesses().get(ServerType.TABLET_SERVER).iterator().next());
Set<TabletMetadata.Location> replStates = new HashSet<>();
@SuppressWarnings("deprecation")
TableId repTable = org.apache.accumulo.core.replication.ReplicationTable.ID;
+
+ while (getZkDeadCount(cluster.getServerContext().getZooReader(),
+ cluster.getServerContext().getInstanceID()) <= deadCount) {
Review Comment:
Does this count ever go back down after increasing? If so, this check could be flaky if it went up and then back down before we checked it here.
##########
test/src/main/java/org/apache/accumulo/test/ManagerRepairsDualAssignmentIT.java:
##########
@@ -150,12 +164,24 @@ public void test() throws Exception {
}
}
+ private int getZkDeadCount(ZooReader zooReader, final InstanceId iid) {
+ String tPath = Constants.ZROOT + "/" + iid + Constants.ZDEADTSERVERS;
+ try {
+ int count = zooReader.getChildren(tPath).size();
+ log.debug("Current dead server count: {}", count);
+ return count;
+ } catch (Exception ex) {
+ throw new IllegalStateException(
+ "Failed to read the number of dead tservers reported in ZooKeeper", ex);
+ }
+ }
+
private void waitForCleanStore(TabletStateStore store) {
while (true) {
try (ClosableIterator<TabletLocationState> iter = store.iterator()) {
iter.forEachRemaining(t -> {});
} catch (Exception ex) {
- System.out.println(ex);
+ log.debug("Exception waiting fro clean store", ex);
Review Comment:
```suggestion
log.debug("Exception waiting for clean store", ex);
```
##########
test/src/main/java/org/apache/accumulo/test/ManagerRepairsDualAssignmentIT.java:
##########
@@ -99,12 +102,21 @@ public void test() throws Exception {
}
}
assertEquals(2, states.size());
+ int deadCount = getZkDeadCount(cluster.getServerContext().getZooReader(),
+ cluster.getServerContext().getInstanceID());
// Kill a tablet server... we don't care which one... wait for everything to be reassigned
cluster.killProcess(ServerType.TABLET_SERVER,
cluster.getProcesses().get(ServerType.TABLET_SERVER).iterator().next());
Set<TabletMetadata.Location> replStates = new HashSet<>();
@SuppressWarnings("deprecation")
TableId repTable = org.apache.accumulo.core.replication.ReplicationTable.ID;
+
+ while (getZkDeadCount(cluster.getServerContext().getZooReader(),
+ cluster.getServerContext().getInstanceID()) <= deadCount) {
+ log.debug("Waiting for dead server to be reported in ZooKeeper. Waiting for next check...");
+ UtilWaitThread.sleep(5000);
+ }
Review Comment:
I think you could use `Wait.waitFor` to watch for this case.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3776: Count dead tservers to continue in ManagerRepairsDualAssignmentIT
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3776:
URL: https://github.com/apache/accumulo/pull/3776#discussion_r1336324480
##########
test/src/main/java/org/apache/accumulo/test/ManagerRepairsDualAssignmentIT.java:
##########
@@ -99,12 +102,21 @@ public void test() throws Exception {
}
}
assertEquals(2, states.size());
+ int deadCount = getZkDeadCount(cluster.getServerContext().getZooReader(),
+ cluster.getServerContext().getInstanceID());
// Kill a tablet server... we don't care which one... wait for everything to be reassigned
cluster.killProcess(ServerType.TABLET_SERVER,
cluster.getProcesses().get(ServerType.TABLET_SERVER).iterator().next());
Set<TabletMetadata.Location> replStates = new HashSet<>();
@SuppressWarnings("deprecation")
TableId repTable = org.apache.accumulo.core.replication.ReplicationTable.ID;
+
+ while (getZkDeadCount(cluster.getServerContext().getZooReader(),
+ cluster.getServerContext().getInstanceID()) <= deadCount) {
+ log.debug("Waiting for dead server to be reported in ZooKeeper. Waiting for next check...");
+ UtilWaitThread.sleep(5000);
+ }
Review Comment:
changed in 4994f630bf
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3776: Count dead tservers to continue in ManagerRepairsDualAssignmentIT
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3776:
URL: https://github.com/apache/accumulo/pull/3776#discussion_r1336325155
##########
test/src/main/java/org/apache/accumulo/test/ManagerRepairsDualAssignmentIT.java:
##########
@@ -150,12 +164,24 @@ public void test() throws Exception {
}
}
+ private int getZkDeadCount(ZooReader zooReader, final InstanceId iid) {
+ String tPath = Constants.ZROOT + "/" + iid + Constants.ZDEADTSERVERS;
+ try {
+ int count = zooReader.getChildren(tPath).size();
+ log.debug("Current dead server count: {}", count);
+ return count;
+ } catch (Exception ex) {
+ throw new IllegalStateException(
+ "Failed to read the number of dead tservers reported in ZooKeeper", ex);
+ }
+ }
+
private void waitForCleanStore(TabletStateStore store) {
while (true) {
try (ClosableIterator<TabletLocationState> iter = store.iterator()) {
iter.forEachRemaining(t -> {});
} catch (Exception ex) {
- System.out.println(ex);
+ log.debug("Exception waiting fro clean store", ex);
Review Comment:
Changed in 4994f630bf
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org