yashmayya commented on code in PR #12984:
URL: https://github.com/apache/kafka/pull/12984#discussion_r1064077119
##########
connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaConfigBackingStore.java:
##########
@@ -723,7 +752,11 @@ private void sendPrivileged(String key, byte[] value) {
try {
fencableProducer.beginTransaction();
- fencableProducer.send(new ProducerRecord<>(topic, key, value));
+ fencableProducer.send(new ProducerRecord<>(topic, key, value),
(metadata, exception) -> {
Review Comment:
I'm not sure I follow what benefit we'd be getting here by handling both the
producer callback error as well as the one thrown by `commitTransaction`? The
control flow would be more straightforward by removing the producer callback
and just relying on `commitTransaction` to throw exceptions, if any. The
producer's Javadoc itself also suggests that callbacks need not be defined when
using the transactional producer since `commitTransaction` will throw the error
from the last failed send in a transaction.
> making them more vague to compensate
We wouldn't be making it more vague. The message would state that the write
to the config topic failed which is the cause for failure. Since the exception
mapper used by Connect's REST server only writes the [top level exception's
message](https://github.com/apache/kafka/blob/d798ec779c25dba31fa5ee9384d159ed54c6e07b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/errors/ConnectExceptionMapper.java#L72)
to the response (i.e. nested exceptions aren't surfaced via the REST API
response), I think it makes sense to keep the top level exception's message
generic and allow users to debug further via the worker logs (where the entire
exception chain's stack trace will be visible). Note that I'm suggesting a
similar change for the non-EOS enabled case as well - i.e. don't use the
producer error directly
[here](https://github.com/apache/kafka/pull/12984/files#diff-c346b7b90fe30ac08b4211375fe36208139a90e40838dd3a7996021a8c4c5b13R1064),
instead wrapping it in a `ConnectExc
eption` which says that the write to the config topic failed. The reasoning
here is that since a Connect user may not even know that Connect uses a
producer under the hood to write certain requests to the config topic for
asynchronous processing, it would make more sense to have an informative
Connect specific exception message rather than directly throwing the producer
exception which may or may not contain enough details to be relevant to a
Connect user.
> If we're hiding the result from the REST calls, are we not also hiding
the error from the herder tick thread?
Hm no, the hiding issue was only for non-EOS enabled workers. Like I've
pointed out above, for workers that have EOS enabled, the REST API does return
a `500` response.
Edit: Another option for the above issue could be changing the exception
mapper to concatenate all the exception messages from the exception chain.
--
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
