-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Refactor IndexingStats.Stats with Builder pattern #19306
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
Refactor IndexingStats.Stats with Builder pattern #19306
Conversation
61caf20 to
e7609b4
Compare
|
❌ Gradle check result for e7609b4: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
sandeshkr419
left a 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.
Thanks @pado0 for working on the changes. The changes look good overall, few comments:
I see you ran into spotless (indenting) failures.
Run ./gradlew :server:spotlessApply to fix these violations and then commit the changes again.
Add a changelog. Refer: https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md
nit: mentioning Resolves <any issue> closes the issue once the PR is merged. For changes which are partially resolving the issue, mention Partially resolves <any issue> to keep the primary issue open. Not something to worry about but I thought to mention it anyway.
server/src/main/java/org/opensearch/index/shard/InternalIndexingStats.java
Outdated
Show resolved
Hide resolved
3259b90 to
a251ef2
Compare
3e3fddd to
2c958be
Compare
|
❌ Gradle check result for 2c958be: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for 2c958be: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
b4fec34 to
b10ecf2
Compare
|
❌ Gradle check result for b10ecf2: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
@peterzhuamazon @gaiksaya Is there a pending change in main causing builds to break? |
|
❌ Gradle check result for b10ecf2: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
b10ecf2 to
1d6a88d
Compare
|
❌ Gradle check result for 1d6a88d: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Jean Kim <bgshhd95@gmail.com>
1d6a88d to
3ef8c9a
Compare
…9306) Signed-off-by: Jean Kim <bgshhd95@gmail.com>
Description
This PR refactors the
IndexingStats.Statsclass to use the Builder pattern instead of relying on multiple constructors.By adopting the Builder pattern, it becomes easier to evolve the stats API, add new metrics, and maintain backward compatibility without forcing disruptive constructor changes.
Based on the related issue:
There are multiple stats-related classes that need similar refactoring, and we are addressing them in priority order. This PR covers IndexingStats.Stats as part of that effort.
Related Issues
Partially resolves #19225
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.