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
Metadata API: report actor runtime status #7040
Metadata API: report actor runtime status #7040
Changes from 30 commits
bb9eb57
5644f7c
b0f08c0
acbc6ad
42c6bb0
7f33edb
9d4a012
81e2d9b
41fd0fa
0795ea8
2c8162b
f3bbe0b
6b68cb3
311ef3b
c71430f
01e5eb7
dd9ba6d
2bdbcc3
1eb1f35
68b5aa2
f7fcb0c
d381443
054a1c6
54f4c29
410b989
40a4861
ae1c8aa
6e7a6ed
5023d2e
753649a
02c751c
3d24b04
6ed4831
2e3f3fa
8023a1f
4a8b73f
8521923
2750607
4218803
5fcb057
57554bc
8360ec9
0704706
657791c
1c13428
83234b1
6405e8a
36c36d2
f412cca
8e608b8
fb542dd
48da34c
d744520
3e49978
9712d15
6e490c8
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.
Just trying to understand the context: Why knowing count would be useful here? Some debugging purpose to know of load?
I see that it is already there but just trying to understand the relevance of 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.
I don't fully understand it either. But it was there before, and I didn't add it - I just added it here too for consistency.
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.
As it is a message received from placement, should the name be more explicit?
placement_msg
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 the "msg" is redundant IMHO. Just sending more bytes over the wire (when using JSON).
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 the actor api level also be included in this ?
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 so, but it’s currently not used. When the placement does report the min actor level, then we can add it.
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.
It concerns me somewhat that the metadata API definition is quite informal being in this package, and is treated in a more ad-hoc way than say the CRDs or proto definitions. Do you think it would be a good idea to "promote" them? Ideally, I think we should move these to proto definitions, which would make for stronger compatibilities for serious consumers of this API.
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 isn't a new change, but regardless:
I don't love it, but we can't change this much.