adixitconfluent commented on code in PR #17957:
URL: https://github.com/apache/kafka/pull/17957#discussion_r1866569428
##########
core/src/main/java/kafka/server/share/SharePartition.java:
##########
@@ -380,6 +380,14 @@ public CompletableFuture<Void> maybeInitialize() {
// Update state to initializing to avoid any concurrent requests
to be processed.
partitionState = SharePartitionState.INITIALIZING;
+ } catch (Exception e) {
+ log.error("Failed to initialize the share partition: {}-{}",
groupId, topicIdPartition, e);
+ completeInitializationWithException(future, e);
+ return future;
+ } finally {
+ lock.writeLock().unlock();
+ }
+ try {
Review Comment:
@chia7712, I can remove the `try catch` from
`persister.readState/persister.writeState` calls and modify the test cases for
the corresponding behaviour. From what I see in the persister code lines
[here](https://github.com/apache/kafka/blob/trunk/share/src/main/java/org/apache/kafka/server/share/persister/DefaultStatePersister.java#L208)
and
[here](https://github.com/apache/kafka/blob/trunk/share/src/main/java/org/apache/kafka/server/share/persister/DefaultStatePersister.java#L118),
`CompletableFuture` is being utilized to propagate the exception. I think the
`try catch` in `persister.readState` was added as an additional check in the
initial iterations of development, but we don't seem to need it now. If we're
all aligned, then I'll make this change and re-raise a review.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
