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
[component] Move component status reporting public API to new componentstatus module #10730
[component] Move component status reporting public API to new componentstatus module #10730
Changes from 5 commits
356a4a5
9654aa1
4132d05
1c35eb7
e40d606
270752d
5a17c56
a5366a6
e0c4f98
fb98b95
6251a9f
b94469a
e99f56d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 would move this + the
InstanceId
to the "extension/statuswatcher` or something like that to be consistent with storage and other type of extensions.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 think you're probably correct, but I'd prefer to encompass all public component status reporting features behind
componentstatus
for now while it is experimental and subject to lots of breaking API discussions. Once we feel better about its API we could move it intoextension/storage
as a final location. Likely other aspects ofcomponentstatus
will move around in the future as well.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.
Not suggesting to
extension/storage
but toextension/statuswatcher
:)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 am confused about why we do this then. Is this to just remove unstable code (that is ok)? But still you should at least document all these things as todos (file issues) and then we can do it in a separate PR.
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.
Removing unstable code from
component
andextension
is the goal. Issue created: #10764There 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.
Timestamp seem to be use-less, because it is always "now", which means consumers (watchers) can also read if they need it.
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.
This is important for the healthcheck extension, which also needs to synthesize events. When synthesizing events the timestamp needs to be settable. We used to do this in core, but have removed it, at least temporarily. I was proposing we add an option to set the timestamp in a followup: #10730 (comment).
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.
What about, if this is needed for that extension, we can have
StatusWithTime struct {Status, Time}
in the healthcheck and not here. No point in having it here.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.
Also, until we discuss the next PR, I would rather remove this for the moment.
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 prefer to keep this PR a 1-to-1 move of stuff related to component status reporting into
componentstatus
. In my opinion we can have API discussions aboutcomponentstatus
in future issues/PRs. I can update the godocs to state that everything incomponentstatus
is experimental and currently exempt from our deprecation process if you'd like.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.
Then update the title to "move code" instead of adding a new module which to me means we can do a better job.
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 think a timestamp will be useful for other status watchers and worth considering for the generated status event. Otherwise, if there are multiple watchers that care about the timestamp, each one will have to wrap the event in a custom struct and generate timestamps independently.
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.
When in doubt leave it out. I am not convinced yet, and until we have 2-3 use-cases we should not add it, since adding later is backwards compatible, but currently we pay the cost of reading the time as well as the maintenance cost.
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.
Followup issue created: #10763
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 will still suggest the timestamp has value. I wrote the following on the issue: