-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Concurrent Searching (Experimental): modify profiling implementation to support concurrent data collection #1673
Conversation
Can one of the admins verify this patch? |
✅ Gradle Wrapper Validation success 5da09665877c900cde88a6a2c0104ce45a22793e |
✅ Gradle Precommit success 5da09665877c900cde88a6a2c0104ce45a22793e |
❌ Gradle Check failure 5da09665877c900cde88a6a2c0104ce45a22793e |
5da0966
to
0f00e7e
Compare
❌ Gradle Check failure 0f00e7e9b6b8131d1d86c6a2236a470ef44c798a |
0f00e7e
to
4158155
Compare
✅ Gradle Check success 41581555c238e033a641fe81538af0f28ac2c162 |
4158155
to
d6456f0
Compare
❌ Gradle Check failure d6456f0e558b6d3800afe0bacbe9213cf0e99751 |
d6456f0
to
8ec07fd
Compare
❌ Gradle Check failure 8ec07fdc50a3506461f6eead5a54acbce1277fb7 |
8ec07fd
to
34bd907
Compare
❌ Gradle Check failure 34bd90715f7d6538a695111891613a3ac59221f4 |
34bd907
to
9cc5d33
Compare
❌ Gradle Check failure 9cc5d33e502f1aa093e4fa52b92867e817f92ab9 |
9cc5d33
to
ac4968b
Compare
✅ Gradle Check success ac4968bc729fc04db6dda20e1f5a321aaf6ed9f9 |
/** | ||
* Build a timing count breakdown for current instance | ||
*/ | ||
protected Map<String, Long> doBuildBreakdownMap() { |
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 is the purpose of introducing this method as opposed to making toBreakdownMap
non-final and overriding it in ConcurrentQueryProfileBreakdown?
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.
Good one, only to not break the API compatibility, this is the only reason, 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.
What would break by making toBreakdownMap
non-final? If there was ever any intent to making that method final, it is being circumvented by this new protected method, so I'd be inclined to simplify things and just make it non-final. Unless there is some compatibility concern that I don't understand.
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.
No objections, will do that, thanks!
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.
Fixed!
|
||
import java.util.Collection; | ||
|
||
public interface InternalProfileComponent { |
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 is the purpose of introducing this interface? I see that only one class implements it now. Is there plan to have multiple implementations 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.
Yes, it is extensively used in #1500 to include CollectorManager
instances into profiling
❌ Gradle Check failure 5df5b27851f1ca016426f08de1cf63e7bde69119 |
❌ Gradle Check failure 1337a0fdfb1efa19297ad41581de3253c4a7212a |
❌ Gradle Check failure 97fa09ca305b4c03cbf90251e60e97b5bd13a54e |
❌ Gradle Check failure 3e0aebda21388cf24b28a6f9f98d3d180c844f1f |
…to support concurrent data collection Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
3e0aebd
to
f88cc2e
Compare
Signed-off-by: Andriy Redko andriy.redko@aiven.io
Description
Part of #1500, it was suggested to split the initial pull request into smaller chunks to simplify the review. This one extracts changes related to profiler support.
Issues Resolved
[List any issues this PR will resolve]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.