You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "keith-turner (via GitHub)" <gi...@apache.org> on 2024/05/22 18:35:11 UTC
[PR] handles failure in compaction planner [accumulo]
keith-turner opened a new pull request, #4586:
URL: https://github.com/apache/accumulo/pull/4586
Updates the CompactionJobGenerator to log failures in planning compactions. This avoids causing problems for the tablet management iterator when its trying to do other things like process tablet location updates.
Added a test that ensures tables with flaky compaction planners can still be read and written. If failed compaction planning were to interfere with tablet assignment this would cause the reads and/or writes to fail.
--
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
Re: [PR] handles failure in compaction planner [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4586:
URL: https://github.com/apache/accumulo/pull/4586#discussion_r1619255689
##########
server/base/src/main/java/org/apache/accumulo/server/compaction/CompactionJobGenerator.java:
##########
@@ -162,23 +166,29 @@ public Map<String,String> getExecutionHints() {
return dispatcher.dispatch(dispatchParams).getService();
}
+ private Level getLevel(TableId tableId, CompactionServiceId serviceId,
Review Comment:
Updated to use EscalatingLogger in 5500afb. Ran the new IT in this PR and looked at the logs and the changes are working. I was wondering if the exception some of the log messages had would be included in the key created from arguments in ConditionalLogger. Poked around a bit and found its not included.
--
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
Re: [PR] handles failure in compaction planner [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #4586:
URL: https://github.com/apache/accumulo/pull/4586#discussion_r1613446675
##########
server/base/src/main/java/org/apache/accumulo/server/compaction/CompactionJobGenerator.java:
##########
@@ -162,23 +166,29 @@ public Map<String,String> getExecutionHints() {
return dispatcher.dispatch(dispatchParams).getService();
}
+ private Level getLevel(TableId tableId, CompactionServiceId serviceId,
Review Comment:
Curious if you can use the EscalatingLogger ( or some other variant of ConditionalLogger) that's included in #4558 .
--
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
Re: [PR] handles failure in compaction planner [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4586:
URL: https://github.com/apache/accumulo/pull/4586#discussion_r1621434478
##########
test/src/main/java/org/apache/accumulo/test/compaction/CompactionExecutorIT.java:
##########
@@ -230,6 +255,35 @@ public void cleanup() {
}
}
+ @Test
+ public void testFailingPlanners() throws Exception {
+ // This test ensures that a table w/ failing compaction planner can still be read and written.
+
+ try (AccumuloClient client = Accumulo.newClient().from(getClientProps()).build()) {
+ createTable(client, "fail1", "cse1");
+ createTable(client, "fail2", "cse2");
+ createTable(client, "fail3", "cse3");
+
+ // ensure tablets can still be assigned and written w/ failing compaction services
+ addFiles(client, "fail1", 30);
+ addFiles(client, "fail2", 30);
+ addFiles(client, "fail3", 30);
+
+ // ensure tablets can still be assigned and scanned w/ failing compaction services
+ assertEquals(30, scanTable(client, "fail1").size());
+ assertEquals(30, scanTable(client, "fail2").size());
+ assertEquals(30, scanTable(client, "fail3").size());
+
+ // compactions should never run on these tables, but sleep a bit to be sure
+ Thread.sleep(2000);
+
+ // do no expect any compactions to run
+ assertEquals(30, getFiles(client, "fail1").size());
+ assertEquals(30, getFiles(client, "fail2").size());
+ assertEquals(30, getFiles(client, "fail3").size());
+ }
+ }
+
Review Comment:
Nice, I have wanted to check logging in unit test in the past but did not know how. Looked around and there is currently no unit test for CompactionJobGenerator. Would need to add one to test logging, could be something to look into as a follow on.
--
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
Re: [PR] handles failure in compaction planner [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner merged PR #4586:
URL: https://github.com/apache/accumulo/pull/4586
--
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
Re: [PR] handles failure in compaction planner [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #4586:
URL: https://github.com/apache/accumulo/pull/4586#issuecomment-2135853487
@keith-turner - you also have PR #4435 and from what I can tell, it's for the same issue. If so, can you close one of them?
--
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
Re: [PR] handles failure in compaction planner [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #4586:
URL: https://github.com/apache/accumulo/pull/4586#discussion_r1621400065
##########
server/base/src/main/java/org/apache/accumulo/server/compaction/CompactionJobGenerator.java:
##########
@@ -162,23 +166,29 @@ public Map<String,String> getExecutionHints() {
return dispatcher.dispatch(dispatchParams).getService();
}
+ private Level getLevel(TableId tableId, CompactionServiceId serviceId,
Review Comment:
I'm wondering if there is a downside to having too many ConditionalLoggers, as there is a Caffeine cache for each one. I wonder if it would be better to have one and add the logger name to the key.
--
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
Re: [PR] handles failure in compaction planner [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #4586:
URL: https://github.com/apache/accumulo/pull/4586#issuecomment-2137789906
> @keith-turner - you also have PR https://github.com/apache/accumulo/pull/4435 and from what I can tell, it's for the same issue. If so, can you close one of them?
I had forgot about that one, was just trying to work down the TODOs in the elasticity branch. Looking at the two PRs this one is better because it adds a test for an erroring planner plugin. It also consolidates a duplicate code pattern. Will close the other one.
--
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
Re: [PR] handles failure in compaction planner [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4586:
URL: https://github.com/apache/accumulo/pull/4586#discussion_r1619162594
##########
server/base/src/main/java/org/apache/accumulo/server/compaction/CompactionJobGenerator.java:
##########
@@ -162,23 +166,29 @@ public Map<String,String> getExecutionHints() {
return dispatcher.dispatch(dispatchParams).getService();
}
+ private Level getLevel(TableId tableId, CompactionServiceId serviceId,
Review Comment:
I can try that
--
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
Re: [PR] handles failure in compaction planner [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #4586:
URL: https://github.com/apache/accumulo/pull/4586#discussion_r1621396847
##########
server/base/src/main/java/org/apache/accumulo/server/compaction/CompactionJobGenerator.java:
##########
@@ -162,23 +166,29 @@ public Map<String,String> getExecutionHints() {
return dispatcher.dispatch(dispatchParams).getService();
}
+ private Level getLevel(TableId tableId, CompactionServiceId serviceId,
Review Comment:
Do you think the exception needs to be in the Key?
--
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
Re: [PR] handles failure in compaction planner [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #4586:
URL: https://github.com/apache/accumulo/pull/4586#discussion_r1621403160
##########
test/src/main/java/org/apache/accumulo/test/compaction/CompactionExecutorIT.java:
##########
@@ -230,6 +255,35 @@ public void cleanup() {
}
}
+ @Test
+ public void testFailingPlanners() throws Exception {
+ // This test ensures that a table w/ failing compaction planner can still be read and written.
+
+ try (AccumuloClient client = Accumulo.newClient().from(getClientProps()).build()) {
+ createTable(client, "fail1", "cse1");
+ createTable(client, "fail2", "cse2");
+ createTable(client, "fail3", "cse3");
+
+ // ensure tablets can still be assigned and written w/ failing compaction services
+ addFiles(client, "fail1", 30);
+ addFiles(client, "fail2", 30);
+ addFiles(client, "fail3", 30);
+
+ // ensure tablets can still be assigned and scanned w/ failing compaction services
+ assertEquals(30, scanTable(client, "fail1").size());
+ assertEquals(30, scanTable(client, "fail2").size());
+ assertEquals(30, scanTable(client, "fail3").size());
+
+ // compactions should never run on these tables, but sleep a bit to be sure
+ Thread.sleep(2000);
+
+ // do no expect any compactions to run
+ assertEquals(30, getFiles(client, "fail1").size());
+ assertEquals(30, getFiles(client, "fail2").size());
+ assertEquals(30, getFiles(client, "fail3").size());
+ }
+ }
+
Review Comment:
FYI, if you wanted to test that the ERROR logging is being emitted (that the escalation is happening), you can look at https://github.com/apache/accumulo/blob/main/core/src/test/java/org/apache/accumulo/core/logging/EscalatingLoggerTest.java. I was able to figure out how to programatically change the logger and capture the output in a Java object that you can then test.
--
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