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
PA: Fixed scheduler manager #88
PA: Fixed scheduler manager #88
Changes from all commits
c326db9
8db6bc6
026b7e4
5176557
a01905c
637583f
50d3358
c79a343
70812f2
b7279ea
b75144d
b81ef1f
8d3f7e0
855f11b
7282e1b
e5ac4b9
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.
This is technically an anti-pattern. This should (1) use
std::make_unique
, and (2) directly store it straight into the output storage.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 gives an error because the constructor is protected and
std::make_unique
calls the constructor from outside of class scope, so for this to work, we should make constructor public.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.
Fiiiiiiiiine. It's okay this way then.
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.
@IzzyPutterman why are the values from schedule (e.g.
--schedule=1,2,3,4,4.5,6,9,10,11,13,15,18
being divided by the request rate?I thought those values are supposed to be absolute timestamps?
cc @lkomali @debermudez
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.
Okay I... am conflicted here, but here's my feedback on what I see.
This sort of API is... not great. It's extremely error prone, due to the sheer number of unnamed parameters that are mixed between input and output. Yet, it matches the current existing API from
RequestRateManager
, and you're just following the existing pattern, so that's not really on you, and the rest of what I'm typing here isn't directed at you, just my general statement on the state of the codebase in general.To explain the problem: what I see here is full of what's now considered bad programming practice, and we should start considering cleaning this code. I'm just not sure if we should start now with this PR, or have a more thorough clean later, as trying to do this now may (1) take a long time for you and (2) look akward if it's not propagated throughout the rest of the codebase. But also, working incrementally towards having a better codebase is a good path forward.
true
andfalse
in the middle of a function call.batch_size
) and unsigned (max_threads
andnum_of_sequences
) values for size types is odd. In general we should use unsigned for anything size-related.measurement_window_ms
, should be better typed to incorporate the time unit, ideally using std::chrono.Now, looking at it a bit more down the code, in
perf_analyzer.cc
, we have a structure already with all the parameters, calledparams_
, which we unpack into all of these arguments... Instead, we could just easily pass that structure around, and let the callees decide what to pick from them. As in, my first point in this feedback is already done, basically, one would just need to haul the structure up and down instead of unpacking / repacking it with so many arguments.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.
To clarify a bit my stance: I am not against smart pointers in general, just that
shared_ptr
are usuallyunique_ptr
in disguise. They are non-committal about the actual ownership of the object, whereasunique_ptr
has a clearer definition 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.
The
shared_ptr
is for client backend factory and I have no idea how much of an effort it is to modify everything tounique_ptr
. I can create a ticket for that effort.Coming to the constructor call, I agree with all your points. As far as I understand, your suggestion is to pass params object instead of the individual arguments. It makes sense to me. Again, I'm not sure if I'm going to break something by modifying the API. Let me know if I should add a ticket to investigate and modify the all the related APIs because as you said, it might look awkward if we modify for some of them and it's not propagated until some time after.
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, let's just try and plumb the parameters object in this one instance, and keep in mind we should do this for newer code. If we have time, we should go back and clean older API incrementally. You're right: let's not break existing code right now, but let's avoid introducing new code which follows this older pattern.
For the unique_ptr vs shared_ptr, this was more of a general remark that we should keep in mind when adding new shared_ptr in the codebase. At some point we should review the existing usage and clean it up, but not in this one PR.