Skip to content

MINOR: Improve EOS example exception handling#8052

Merged
hachikuji merged 8 commits intoapache:trunkfrom
abbccdda:no_exception_catch
Feb 20, 2020
Merged

MINOR: Improve EOS example exception handling#8052
hachikuji merged 8 commits intoapache:trunkfrom
abbccdda:no_exception_catch

Conversation

@abbccdda
Copy link
Copy Markdown

@abbccdda abbccdda commented Feb 6, 2020

The current EOS example is over-complicating the exception handling by mixing non fatal and fatal ones. This cleanup is trying to make the code readability better. Will rebase after #8051 is merged

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the only case that we can abort a transaction. This only handles offset commit failures, but what about send failures?

Copy link
Copy Markdown
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating this. Left a few comments.

Comment thread examples/src/main/java/kafka/examples/ExactlyOnceMessageProcessor.java Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is InvalidOffsetException fatal?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it could be retriable.

Comment thread examples/src/main/java/kafka/examples/ExactlyOnceMessageProcessor.java Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we do "group mode" only in this example? The example doesn't really extend to multiple instances otherwise.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I follow, we could manually compute the partitions to assign to for the standalone mode?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. Is it actually safe to abort following a TimeoutException? I think this would cause an illegal state error in the producer. To handle this correctly, the application should retry the commit.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we shouldn't handle the timeout, as we always rely on max_block to ensure the request is successful?

@abbccdda abbccdda force-pushed the no_exception_catch branch 4 times, most recently from 0996e26 to e84165a Compare February 18, 2020 20:09
Copy link
Copy Markdown
Contributor

@hachikuji hachikuji Feb 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the purpose of understanding EOS, the main exceptions that are worth calling out are ProducerFencedException and FencedInstanceIdException. I would suggest we write the example like this:

try {
   ...
   producer.commitTransaction;
} catch (ProducerFencedException e) {
  throw KafkaException("The transactional.id $transactionalId has been claimed by another process");
} catch (FencedInstanceIdException e) {
  throw KafkaException("The group.instance.id $instanceId has been claimed by another process");
} catch (KafkaException e) {
  // If we have not been fenced, try to abort the transaction and continue. This will raise immediately
  // if the producer has hit a fatal error.
  producer.abortTransaction();
}

Comment thread examples/src/main/java/kafka/examples/ExactlyOnceMessageProcessor.java Outdated
Comment thread examples/src/main/java/kafka/examples/ExactlyOnceMessageProcessor.java Outdated
@hachikuji
Copy link
Copy Markdown
Contributor

retest this please

Copy link
Copy Markdown
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for simplifying this example.

@hachikuji
Copy link
Copy Markdown
Contributor

retest this please

@hachikuji
Copy link
Copy Markdown
Contributor

ok to test

@hachikuji hachikuji merged commit 776565f into apache:trunk Feb 20, 2020
guozhangwang pushed a commit that referenced this pull request Feb 21, 2020
The current EOS example mixes fatal and non-fatal error handling. This patch fixes this problem and simplifies the example.

Reviewers: Jason Gustafson <jason@confluent.io>
@guozhangwang
Copy link
Copy Markdown
Contributor

Cherry-picked to 2.5

@abbccdda abbccdda deleted the no_exception_catch branch February 21, 2020 19:06
ijuma added a commit to confluentinc/kafka that referenced this pull request Feb 24, 2020
* apache-github/trunk: (23 commits)
  KAFKA-9530; Fix flaky test `testDescribeGroupWithShortInitializationTimeout` (apache#8154)
  HOTFIX: fix NPE in Kafka Streams IQ (apache#8158)
  MINOR: set scala version automatically based on gradle.properties
  KAFKA-9577; SaslClientAuthenticator incorrectly negotiates SASL_HANDSHAKE version (apache#8142)
  KAFKA-9441: Add internal TransactionManager (apache#8105)
  MINOR: Document endpoints for connector topic tracking (KIP-558)
  MINOR: Standby task commit needed when offsets updated (apache#8146)
  KAFKA-9206; Throw KafkaException on CORRUPT_MESSAGE error in Fetch response (apache#8111)
  MINOR: Remove unwanted regexReplace on tests/kafkatest/__init__.py
  KAFKA-9586: Fix errored json filename in ops documentation
  KAFKA-9575: Mention ZooKeeper 3.5.7 upgrade
  KAFKA-9481: Graceful handling TaskMigrated and TaskCorrupted (apache#8058)
  HOTFIX: don't try to remove uninitialized changelogs from assignment & don't prematurely mark task closed (apache#8140)
  MINOR: Fix javadoc at org.apache.kafka.clients.producer.KafkaProducer.InterceptorCallback#onCompletion (apache#7337)
  MINOR: Improve EOS example exception handling (apache#8052)
  MINOR: Fix a number of warnings in clients test (apache#8073)
  MINOR: Update shell scripts to support z/OS system (apache#7913)
  MINOR: Wording fix in Streams DSL docs (apache#5692)
  MINOR: Add missing @test annotation to MetadataTest#testMetadataMerge (apache#8141)
  KAFKA-9533: ValueTransform forwards `null` values (apache#8108)
  ...
ijuma added a commit to confluentinc/kafka that referenced this pull request Feb 24, 2020
…etrics-common

* confluent/master: (76 commits)
  KAFKA-9530; Fix flaky test `testDescribeGroupWithShortInitializationTimeout` (apache#8154)
  HOTFIX: fix NPE in Kafka Streams IQ (apache#8158)
  MINOR: set scala version automatically based on gradle.properties
  KAFKA-9577; SaslClientAuthenticator incorrectly negotiates SASL_HANDSHAKE version (apache#8142)
  KAFKA-9441: Add internal TransactionManager (apache#8105)
  MINOR: Document endpoints for connector topic tracking (KIP-558)
  MINOR: Standby task commit needed when offsets updated (apache#8146)
  Changes to migrate to Artifactory (#263)
  KAFKA-9206; Throw KafkaException on CORRUPT_MESSAGE error in Fetch response (apache#8111)
  MINOR: Remove unwanted regexReplace on tests/kafkatest/__init__.py
  KAFKA-9586: Fix errored json filename in ops documentation
  KAFKA-9575: Mention ZooKeeper 3.5.7 upgrade
  KAFKA-9481: Graceful handling TaskMigrated and TaskCorrupted (apache#8058)
  HOTFIX: don't try to remove uninitialized changelogs from assignment & don't prematurely mark task closed (apache#8140)
  MINOR: Fix javadoc at org.apache.kafka.clients.producer.KafkaProducer.InterceptorCallback#onCompletion (apache#7337)
  MINOR: Improve EOS example exception handling (apache#8052)
  MINOR: Fix a number of warnings in clients test (apache#8073)
  MINOR: Update shell scripts to support z/OS system (apache#7913)
  MINOR: Wording fix in Streams DSL docs (apache#5692)
  MINOR: Add missing @test annotation to MetadataTest#testMetadataMerge (apache#8141)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants