You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cassandra.apache.org by "yifan-c (via GitHub)" <gi...@apache.org> on 2024/11/07 23:57:14 UTC
[PR] CASSANDRA-20066: Expose detailed bulk write failure message for better insight [cassandra-analytics]
yifan-c opened a new pull request, #92:
URL: https://github.com/apache/cassandra-analytics/pull/92
Patch by Yifan Cai; Reviewed by TBD for CASSANDRA-20066
--
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: commits-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org
Re: [PR] CASSANDRA-20066: Expose detailed bulk write failure message for better insight [cassandra-analytics]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #92:
URL: https://github.com/apache/cassandra-analytics/pull/92#discussion_r1844256615
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/bulkwriter/BulkWriteValidator.java:
##########
@@ -110,19 +112,24 @@ public static void updateFailureHandler(CommitResult commitResult,
});
}
- private static void logFailedRanges(Logger logger, String phase,
- List<ReplicaAwareFailureHandler<RingInstance>.ConsistencyFailurePerRange> failedRanges)
+ // aggregate the stream errors in order to provide a better insight on failure
+ private static String aggregateErrors(List<ReplicaAwareFailureHandler<RingInstance>.ConsistencyFailurePerRange> failedRanges)
{
+ StringBuilder sb = new StringBuilder();
for (ReplicaAwareFailureHandler<RingInstance>.ConsistencyFailurePerRange failedRange : failedRanges)
{
- failedRange.failuresPerInstance.forEachInstance((instance, errors) -> {
- logger.error("Failed in phase {} for {} on {}. Failure: {}",
Review Comment:
The logging is moved to `validateClOrFail`. However, I should probably not truncate the message here. Making an update.
--
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: commits-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org
Re: [PR] CASSANDRA-20066: Expose detailed bulk write failure message for better insight [cassandra-analytics]
Posted by "JeetKunDoug (via GitHub)" <gi...@apache.org>.
JeetKunDoug commented on code in PR #92:
URL: https://github.com/apache/cassandra-analytics/pull/92#discussion_r1844118718
##########
scripts/build-dtest-jars.sh:
##########
@@ -52,7 +52,7 @@ else
if [[ "$CLEAN" == "true" ]]; then
echo "Clean up $DTEST_JAR_DIR"
rm -rf "$DTEST_JAR_DIR/cassandra-build"
- rm "$DTEST_JAR_DIR/dtest*.jar"
+ rm -rf "$DTEST_JAR_DIR/dtest*.jar"
Review Comment:
Why? you should only be deleting the jar _files_ so what does `-r` do here?
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/bulkwriter/BulkWriteValidator.java:
##########
@@ -110,19 +112,24 @@ public static void updateFailureHandler(CommitResult commitResult,
});
}
- private static void logFailedRanges(Logger logger, String phase,
- List<ReplicaAwareFailureHandler<RingInstance>.ConsistencyFailurePerRange> failedRanges)
+ // aggregate the stream errors in order to provide a better insight on failure
+ private static String aggregateErrors(List<ReplicaAwareFailureHandler<RingInstance>.ConsistencyFailurePerRange> failedRanges)
{
+ StringBuilder sb = new StringBuilder();
for (ReplicaAwareFailureHandler<RingInstance>.ConsistencyFailurePerRange failedRange : failedRanges)
{
- failedRange.failuresPerInstance.forEachInstance((instance, errors) -> {
- logger.error("Failed in phase {} for {} on {}. Failure: {}",
Review Comment:
Not thrilled with the fact that we lost the logging completely - I'd build the error message/cap its length, but continue logging everything, rather than just building the error and returning it.
##########
cassandra-analytics-integration-tests/src/test/java/org/apache/cassandra/analytics/correctness/BulkWriteCorruptionTest.java:
##########
@@ -53,22 +56,55 @@
import static org.apache.cassandra.testing.TestUtils.ROW_COUNT;
import static org.apache.cassandra.testing.TestUtils.TEST_KEYSPACE;
import static org.assertj.core.api.Assertions.assertThat;
-import static org.assertj.core.api.Assertions.fail;
-public class BulkWriteDiskCorruptionTest extends SharedClusterSparkIntegrationTestBase
+public class BulkWriteCorruptionTest extends SharedClusterSparkIntegrationTestBase
{
- private static final QualifiedName QUALIFIED_NAME = new QualifiedName(TEST_KEYSPACE, "test_write_disk_corruption");
+ enum CorruptionMode
+ {
+ DISK,
+ WIRE
+ }
+
+ private static final QualifiedName QUALIFIED_NAME = new QualifiedName(TEST_KEYSPACE, "test_write_corruption");
static
{
- // Intercepts SortedSSTableWriter#validateSSTables to corrupt file on purpose.
// Install the class rebase the earliest, before JVM loads the class
- BBHelperInterceptSortedSSTableWriterValidateSSTables.install();
+ BBHelperFileCorrupter.install();
Review Comment:
NIT: Why not just have 2 different byte buddy helper classes, rather than introducing the mode enum/if statements in two different intercepted methods?
--
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: commits-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org
Re: [PR] CASSANDRA-20066: Expose detailed bulk write failure message for better insight [cassandra-analytics]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #92:
URL: https://github.com/apache/cassandra-analytics/pull/92#discussion_r1844268469
##########
scripts/build-dtest-jars.sh:
##########
@@ -52,7 +52,7 @@ else
if [[ "$CLEAN" == "true" ]]; then
echo "Clean up $DTEST_JAR_DIR"
rm -rf "$DTEST_JAR_DIR/cassandra-build"
- rm "$DTEST_JAR_DIR/dtest*.jar"
+ rm -rf "$DTEST_JAR_DIR/dtest*.jar"
Review Comment:
Because it failed to remove files. I got this error.
```
+ rm '.../cassandra-analytics/dependencies/dtest*.jar'
rm: .../cassandra-analytics/dependencies/dtest*.jar: No such file or directory
```
However, adding `-rf` is not the correct fix. It just hides the error.
The correct `rm` command is `rm "$DTEST_JAR_DIR"/dtest*.jar`
Thanks for bringing it up!
--
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: commits-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org
Re: [PR] CASSANDRA-20066: Expose detailed bulk write failure message for better insight [cassandra-analytics]
Posted by "JeetKunDoug (via GitHub)" <gi...@apache.org>.
JeetKunDoug commented on code in PR #92:
URL: https://github.com/apache/cassandra-analytics/pull/92#discussion_r1844420731
##########
cassandra-analytics-integration-tests/src/test/java/org/apache/cassandra/analytics/correctness/BulkWriteCorruptionTest.java:
##########
@@ -53,22 +56,55 @@
import static org.apache.cassandra.testing.TestUtils.ROW_COUNT;
import static org.apache.cassandra.testing.TestUtils.TEST_KEYSPACE;
import static org.assertj.core.api.Assertions.assertThat;
-import static org.assertj.core.api.Assertions.fail;
-public class BulkWriteDiskCorruptionTest extends SharedClusterSparkIntegrationTestBase
+public class BulkWriteCorruptionTest extends SharedClusterSparkIntegrationTestBase
{
- private static final QualifiedName QUALIFIED_NAME = new QualifiedName(TEST_KEYSPACE, "test_write_disk_corruption");
+ enum CorruptionMode
+ {
+ DISK,
+ WIRE
+ }
+
+ private static final QualifiedName QUALIFIED_NAME = new QualifiedName(TEST_KEYSPACE, "test_write_corruption");
static
{
- // Intercepts SortedSSTableWriter#validateSSTables to corrupt file on purpose.
// Install the class rebase the earliest, before JVM loads the class
- BBHelperInterceptSortedSSTableWriterValidateSSTables.install();
+ BBHelperFileCorrupter.install();
Review Comment:
I think there may be a way around this, but it seems like it requires some more knowledge of ByteBuddy, and I don't have that knowledge right now, so I don't want to hold up this PR.
--
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: commits-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org
Re: [PR] CASSANDRA-20066: Expose detailed bulk write failure message for better insight [cassandra-analytics]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #92:
URL: https://github.com/apache/cassandra-analytics/pull/92#discussion_r1843071242
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/bulkwriter/BulkWriteValidator.java:
##########
@@ -110,19 +112,24 @@ public static void updateFailureHandler(CommitResult commitResult,
});
}
- private static void logFailedRanges(Logger logger, String phase,
- List<ReplicaAwareFailureHandler<RingInstance>.ConsistencyFailurePerRange> failedRanges)
+ // aggregate the stream errors in order to provide a better insight on failure
+ private static String aggregateErrors(List<ReplicaAwareFailureHandler<RingInstance>.ConsistencyFailurePerRange> failedRanges)
{
+ StringBuilder sb = new StringBuilder();
for (ReplicaAwareFailureHandler<RingInstance>.ConsistencyFailurePerRange failedRange : failedRanges)
{
- failedRange.failuresPerInstance.forEachInstance((instance, errors) -> {
- logger.error("Failed in phase {} for {} on {}. Failure: {}",
- phase,
- failedRange.range,
- instance.toString(),
- errors);
- });
+ for (Map.Entry<RingInstance, Collection<String>> entry : failedRange.failuresPerInstance.entrySet())
+ {
+ sb.append("Instance: ").append(entry.getKey()).append(" All failures: ").append(entry.getValue());
Review Comment:
should we add line breaks?
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/bulkwriter/BulkWriteValidator.java:
##########
@@ -110,19 +112,27 @@ public static void updateFailureHandler(CommitResult commitResult,
});
}
- private static void logFailedRanges(Logger logger, String phase,
- List<ReplicaAwareFailureHandler<RingInstance>.ConsistencyFailurePerRange> failedRanges)
+ // aggregate the stream errors in order to provide a better insight on failure
+ private static String logEachErrorAndAggregate(Logger logger,
+ String phase,
+ List<ReplicaAwareFailureHandler<RingInstance>.ConsistencyFailurePerRange> failedRanges)
{
+ StringBuilder sb = new StringBuilder();
for (ReplicaAwareFailureHandler<RingInstance>.ConsistencyFailurePerRange failedRange : failedRanges)
{
- failedRange.failuresPerInstance.forEachInstance((instance, errors) -> {
- logger.error("Failed in phase {} for {} on {}. Failure: {}",
- phase,
- failedRange.range,
- instance.toString(),
- errors);
- });
+ for (Map.Entry<RingInstance, Collection<String>> entry : failedRange.failuresPerInstance.entrySet())
+ {
+ logger.error("Failed in phase {} for {} on {}. Failure: {}", phase, failedRange.range, entry.getKey(), entry.getValue());
+ if (sb.length() > ERROR_MESSAGE_MAX_LENGTH)
Review Comment:
should be >=
--
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: commits-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org
Re: [PR] CASSANDRA-20066: Expose detailed bulk write failure message for better insight [cassandra-analytics]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c merged PR #92:
URL: https://github.com/apache/cassandra-analytics/pull/92
--
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: commits-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org
Re: [PR] CASSANDRA-20066: Expose detailed bulk write failure message for better insight [cassandra-analytics]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #92:
URL: https://github.com/apache/cassandra-analytics/pull/92#discussion_r1844270531
##########
cassandra-analytics-integration-tests/src/test/java/org/apache/cassandra/analytics/correctness/BulkWriteCorruptionTest.java:
##########
@@ -53,22 +56,55 @@
import static org.apache.cassandra.testing.TestUtils.ROW_COUNT;
import static org.apache.cassandra.testing.TestUtils.TEST_KEYSPACE;
import static org.assertj.core.api.Assertions.assertThat;
-import static org.assertj.core.api.Assertions.fail;
-public class BulkWriteDiskCorruptionTest extends SharedClusterSparkIntegrationTestBase
+public class BulkWriteCorruptionTest extends SharedClusterSparkIntegrationTestBase
{
- private static final QualifiedName QUALIFIED_NAME = new QualifiedName(TEST_KEYSPACE, "test_write_disk_corruption");
+ enum CorruptionMode
+ {
+ DISK,
+ WIRE
+ }
+
+ private static final QualifiedName QUALIFIED_NAME = new QualifiedName(TEST_KEYSPACE, "test_write_corruption");
static
{
- // Intercepts SortedSSTableWriter#validateSSTables to corrupt file on purpose.
// Install the class rebase the earliest, before JVM loads the class
- BBHelperInterceptSortedSSTableWriterValidateSSTables.install();
+ BBHelperFileCorrupter.install();
Review Comment:
Yep. It sounds better.
--
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: commits-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org
Re: [PR] CASSANDRA-20066: Expose detailed bulk write failure message for better insight [cassandra-analytics]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #92:
URL: https://github.com/apache/cassandra-analytics/pull/92#discussion_r1844294840
##########
cassandra-analytics-integration-tests/src/test/java/org/apache/cassandra/analytics/correctness/BulkWriteCorruptionTest.java:
##########
@@ -53,22 +56,55 @@
import static org.apache.cassandra.testing.TestUtils.ROW_COUNT;
import static org.apache.cassandra.testing.TestUtils.TEST_KEYSPACE;
import static org.assertj.core.api.Assertions.assertThat;
-import static org.assertj.core.api.Assertions.fail;
-public class BulkWriteDiskCorruptionTest extends SharedClusterSparkIntegrationTestBase
+public class BulkWriteCorruptionTest extends SharedClusterSparkIntegrationTestBase
{
- private static final QualifiedName QUALIFIED_NAME = new QualifiedName(TEST_KEYSPACE, "test_write_disk_corruption");
+ enum CorruptionMode
+ {
+ DISK,
+ WIRE
+ }
+
+ private static final QualifiedName QUALIFIED_NAME = new QualifiedName(TEST_KEYSPACE, "test_write_corruption");
static
{
- // Intercepts SortedSSTableWriter#validateSSTables to corrupt file on purpose.
// Install the class rebase the earliest, before JVM loads the class
- BBHelperInterceptSortedSSTableWriterValidateSSTables.install();
+ BBHelperFileCorrupter.install();
Review Comment:
Made the change and I realized why having the switch.
Each test case only expect corruption at specific locations. Without the switch, `testDataTransferCorruption` has the files corrupted on disk and 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: commits-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org