-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Have metadata tailer use its own thread for processing #7211
Conversation
@@ -69,28 +106,13 @@ public void processRequest(Message<byte[]> msg) { | |||
serviceRequest = ServiceRequest.parseFrom(msg.getData()); | |||
} catch (IOException e) { | |||
log.error("Received bad service request at message {}", msg.getMessageId(), e); | |||
// TODO: find a better way to handle bad request | |||
throw new RuntimeException(e); | |||
errorNotifier.triggerError(e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just remove the try-catch and let the method throw an exception? Everything is caught in the thread anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would still need to differentiate Interrupted vs others. And if we do encounter some exception isn;'t it better to trigger the errorNotifier and exit perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would still need to differentiate Interrupted vs others.
I don't quite follow, the code already differentiates interrupted exceptions vs other exceptions.
Triggering the error in the catch all block in the thread allows us to exit the thread as well and stop further processing of requests. While this doesn't hurt but is cleaner in a sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I misunderstood your comments. Yes, that makes sense. Fixed
throws PulsarClientException { | ||
this.functionMetaDataManager = functionMetaDataManager; | ||
this.reader = reader; | ||
this.reader = readerBuilder | ||
.topic(workerConfig.getFunctionMetadataTopic()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also set the subscription prefix:
subscriptionRolePrefix(workerConfig.getWorkerId() + "-function-metadata-manager")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
* Have metadata tailer use its own thread for processing * Merged with master * Address comments * Address comments Co-authored-by: Sanjeev Kulkarni <sanjeevk@splunk.com>
* Have metadata tailer use its own thread for processing * Merged with master * Address comments * Address comments Co-authored-by: Sanjeev Kulkarni <sanjeevk@splunk.com>
* Have metadata tailer use its own thread for processing * Merged with master * Address comments * Address comments Co-authored-by: Sanjeev Kulkarni <sanjeevk@splunk.com>
needs to cherry-pick to 2.6.2. |
* Have metadata tailer use its own thread for processing * Merged with master * Address comments * Address comments Co-authored-by: Sanjeev Kulkarni <sanjeevk@splunk.com> (cherry picked from commit e64d951)
(If this PR fixes a github issue, please add
Fixes #<xyz>
.)Fixes #
(or if this PR is one task of a github issue, please add
Master Issue: #<xyz>
to link to the master issue.)Master Issue: #
Motivation
Following up on #7180, this pr does the same thing for function metadata tailer.
Modifications
Describe the modifications you've done.
Verifying this change
(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Does this pull request potentially affect one of the following parts:
If
yes
was chosen, please highlight the changesDocumentation