You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "cmccabe (via GitHub)" <gi...@apache.org> on 2023/04/27 17:59:01 UTC
[GitHub] [kafka] cmccabe opened a new pull request, #13651: MINOR: remove spurious call to fatalFaultHandler
cmccabe opened a new pull request, #13651:
URL: https://github.com/apache/kafka/pull/13651
Remove a spurious call to fatalFaultHandler accidentally introduced by KAFKA-14805. We should only invoke the fatal fault handller if we are unable to generate the activation records. If we are unable to write the activation records, a controller failover should be sufficient to remedy the situation.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mumrah commented on a diff in pull request #13651: MINOR: remove spurious call to fatalFaultHandler
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah commented on code in PR #13651:
URL: https://github.com/apache/kafka/pull/13651#discussion_r1179565744
##########
metadata/src/main/java/org/apache/kafka/controller/QuorumController.java:
##########
@@ -1275,12 +1271,21 @@ public static List<ApiMessageAndVersion> generateActivationRecords(
}
return records;
}
+
class CompleteActivationEvent implements ControllerWriteOperation<Void> {
@Override
public ControllerResult<Void> generateRecordsAndResult() {
- List<ApiMessageAndVersion> records = generateActivationRecords(log, logReplayTracker.empty(),
- zkMigrationEnabled, bootstrapMetadata, featureControl);
- return ControllerResult.atomicOf(records, null);
+ try {
+ List<ApiMessageAndVersion> records = generateActivationRecords(log,
+ logReplayTracker.empty(),
+ zkMigrationEnabled,
+ bootstrapMetadata,
+ featureControl);
+ return ControllerResult.atomicOf(records, null);
+ } catch (Throwable t) {
+ throw fatalFaultHandler.handleFault("exception while completing controller " +
Review Comment:
I think in most other places, we are not inside an event when we call the fault handler in QuorumController. I guess that doesn't matter since it calls `Exit.halt` synchronously?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon merged pull request #13651: MINOR: remove spurious call to fatalFaultHandler
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon merged PR #13651:
URL: https://github.com/apache/kafka/pull/13651
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on pull request #13651: MINOR: remove spurious call to fatalFaultHandler
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on PR #13651:
URL: https://github.com/apache/kafka/pull/13651#issuecomment-1526106786
Thanks to @showuon for finding this and posting an initial fix here for discussion : #13647
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a diff in pull request #13651: MINOR: remove spurious call to fatalFaultHandler
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on code in PR #13651:
URL: https://github.com/apache/kafka/pull/13651#discussion_r1179715217
##########
metadata/src/main/java/org/apache/kafka/controller/QuorumController.java:
##########
@@ -1275,12 +1271,21 @@ public static List<ApiMessageAndVersion> generateActivationRecords(
}
return records;
}
+
class CompleteActivationEvent implements ControllerWriteOperation<Void> {
@Override
public ControllerResult<Void> generateRecordsAndResult() {
- List<ApiMessageAndVersion> records = generateActivationRecords(log, logReplayTracker.empty(),
- zkMigrationEnabled, bootstrapMetadata, featureControl);
- return ControllerResult.atomicOf(records, null);
+ try {
+ List<ApiMessageAndVersion> records = generateActivationRecords(log,
+ logReplayTracker.empty(),
+ zkMigrationEnabled,
+ bootstrapMetadata,
+ featureControl);
+ return ControllerResult.atomicOf(records, null);
+ } catch (Throwable t) {
+ throw fatalFaultHandler.handleFault("exception while completing controller " +
Review Comment:
Yes, it's OK to halt from an event (or more generally to invoke fault handlers from that context)
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #13651: MINOR: remove spurious call to fatalFaultHandler
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on PR #13651:
URL: https://github.com/apache/kafka/pull/13651#issuecomment-1526882158
Failed tests are unrelated:
```
Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationTransactionsTest.testOffsetTranslationBehindReplicationFlow()
Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationTransactionsTest.testOneWayReplicationWithAutoOffsetSync()
Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationTransactionsTest.testOneWayReplicationWithFrequentOffsetSyncs()
Build / JDK 8 and Scala 2.12 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationTransactionsTest.testNoCheckpointsIfNoRecordsAreMirrored()
Build / JDK 8 and Scala 2.12 / org.apache.kafka.connect.integration.ExactlyOnceSourceIntegrationTest.testConnectorBoundary
```
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org