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
[salvo] Initial commit for benchmark abstraction framework #73
[salvo] Initial commit for benchmark abstraction framework #73
Changes from 1 commit
dadcf0c
6bc1487
f5d5f8d
75e98e3
d0c7a5b
d5558a8
8dfffa1
e42a89c
8a62d02
3d5b455
2820352
8ea0e61
00e4202
5a4dce3
3f34d06
822c0b1
05bea79
ba882b6
5e6da81
1788e20
5129766
35a26a0
22baac5
81cd018
79b2cbe
4291ade
c5f8f2c
2392ae3
5c26812
73d0dc0
30d2d5d
4f072a9
84bd1af
975cf35
1f207ab
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.
Note: we may need to update these examples after we perform some of the suggested proto field renames.
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.
Right. Certainly doable.
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 we add a top level file comment to each of these proto files? A single sentence summarizing what should the file contain would be enough and would help future contributors.
We can skip this if the file only contains a single message, since the message comment achieves the same in such case.
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.
(optional) Fo consistency - should we rename this file to a matching name (job_control.proto instead of just control.proto). This can help code readers to find the message.
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 can think forward and try to improve readability of the code that will be using these messages. How about we rename this to
is_remote
or even a bit more verboseis_remote_execution
?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 we add comments explaining these types of benchmarks and the implications of setting them? As it stands I don't really understand what we mean by them.
(optional) Since we are only planning to support a smaller subset of execution modes to start with, we can also consider dropping the unsupported one from this initial commit.
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.
Since fields in the code are accessed through the field name, we can make the resulting code more readable if we rename this to
source_repository
. Generally we have no reason for brevity in the API, more readable names make the code self-documenting.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.
Same comment here, can we rename the field to
docker_images
?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.
And here to
environment_vars
.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.
(fyi) This file is a good case for us not having a single BUILD target for all these proto files. None of the other proto files seem to be importing this one, so having a separate compilation unit for it makes sense from the code perspective.
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 probably lack of knowledge on my end, but the names "bind" and "mode" don't help me much to understand the fields. Can we include examples of what they may contain? Once I see that I may be able to suggest better names.
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.
Do we have a good reason to use a map? Or could we just go with a repeated field of a message that includes both the string (I am guessing it is a name) and the volume properties?
Proto maps are a cool feature that helps to optimize code. I.e. we would use it if there are hundreds of volumes and we need to quickly look one up by name. If we are dealing with a much smaller set, we should not try to optimize prematurely since it comes with a cost. The serialization order of proto maps is undefined so this will make our life harder in any sort of unit tests and integration tests we will end up writing.
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.
The field name suggests that the volume contains a set of volumes? Is that the architecture?
Or is it that the volume contains a repeated set of volume properties? In which case we may want to call the field
volume_properties
.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.
For code readability consider longer names, since only the field name will appear in the code:
Example:
ipv4_only
,ipv6_only
,dual_stack
Or similar. Currently seeing the name
all
in code won't give much context.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.
Do we plan to have an execution mode where Envoy won't be started? Can you help me understand our plan around that?
(optional) Secondly it feels we are abusing field by giving its empty value a special meaning. It may be more explicit and readable if we have a separate explicit field, say a boolean called
include_envoy
.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.
Same comment about proto maps here, unless we are optimizing, let's prefer repeated fields.
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.
Possible typo: Captures ...
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 do we mean by "specified source location"? How does one specify 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 really understand the comment as it stands. "This should be implicit." sounds like a wish and I am not sure in what context we are making the wish and why it isn't under our control.
Maybe we can improve the comment by instead focusing on describing the meaning of the proto field?
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 seem to spell out
nighthawk
in most names except here. Can we change this name toreuse_nighthawk_images
instead? It is a bit longer, but longer more explicit names help onboarding and improve code readability.