Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Gh 3340 #3347
Gh 3340 #3347
Changes from 13 commits
e6b76fe
fed20bd
873c024
c440b19
d3a16a6
d9e38d9
50f4157
b530d65
f54b067
e92a48e
4be30c2
8c14729
06a4508
cb8ed16
35b9e1d
2bb6200
f88dfb6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 wonder if we really should emit this event here unconditionally.
Looks like in case of
Reason.AUTH
we restart the container immediately.Therefore we go to the state when children are running, so
ConcurrentContainerStoppedEvent
is loosing its purpose.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.
ConcurrentContainer restart happens only when all the containers are stopped. Whenever all the containers are stopped, then as per the contract ConcurrentContainerStopped event has to be published.
Please give your comments.
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.
That's true, but are they really stopped if we restart it immediately?
That's my concern if we should rethink this situation and don't emit that
ConcurrentContainerStoppedEvent
if we restart immediately.According to your original request about stopping the whole application context on that event, such a restart might cause some problem.
So, why don't we change the logic to be sure that this is exactly a moment for
ConcurrentContainerStoppedEvent
?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 got your point. I am good to make change so that
ConcurrentContainerStoppedEvent
is not published when AUTH error occurred. But, I am having one concern and one suggestion.Concern:
Whatever may be the scenario, as per the contract it is good to publish
ConcurrentContainerStoppedEvent
when all the containers are stopped.Suggestion:
Error reason could be added to
ConcurrentContainerStoppedEvent
, so that application user would take a right decision.public ConcurrentContainerStoppedEvent(Object source, Reason reason)
Please give your idea.
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.
Yes, childStopped API will be called only when the container stopped.
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.
Yes, that sounds good to have that
Reason
propagated down to theConcurrentContainerStoppedEvent
.Thank you!
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.
shall we propagate all reasons for each container as below. As of now, only Auth reason is propagated. Is that sufficient.
public ConcurrentContainerStoppedEvent(Object source, ConsumerStoppedEvent.Reason[] reason) -- all reasons will be propagated.
or
public ConcurrentContainerStoppedEvent(Object source, Reason reason) -- only Auth reason propagated. Other will not be propagated.
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.
If we propagate only
Reason.AUTH
, then no reason in thisreason
property for the event.I don't think we have to propagate all of them.
The logic is like this:
So, any first reason is going to be use (or AUTH) when it overrides the value of that property.