-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
chore: tracking resource more readable #7267
Conversation
Gradle Check (Jenkins) Run Completed with:
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #7267 +/- ##
============================================
+ Coverage 71.10% 71.13% +0.02%
- Complexity 58070 58084 +14
============================================
Files 4824 4824
Lines 273918 273913 -5
Branches 39918 39917 -1
============================================
+ Hits 194768 194844 +76
+ Misses 62802 62723 -79
+ Partials 16348 16346 -2 ☔ View full report in Codecov by Sentry. |
This PR is stalled because it has been open for 30 days with no activity. Remove stalled label or comment or this will be closed in 7 days. |
This PR was closed because it has been stalled for 7 days with no activity. |
Apologies. This PR was auto closed without reaching a resolution from the maintainers. |
Compatibility status:Checks if related components are compatible with change ca4ba2c Incompatible componentsSkipped componentsCompatible componentsCompatible components: [https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/custom-codecs.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/neural-search.git] |
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: fudongying <[email protected]>
b51f074
to
ca4ba2c
Compare
Gradle Check (Jenkins) Run Completed with:
|
Hi @fudongyingluck, the PR is stalled. Is this being worked upon? Feel free to reach out to maintainers for further reviews. |
@@ -564,19 +564,15 @@ public int decrementResourceTrackingThreads() { | |||
int count = numActiveResourceTrackingThreads.decrementAndGet(); | |||
|
|||
if (count == 0) { | |||
List<Exception> listenerExceptions = new ArrayList<>(); | |||
resourceTrackingCompletionListeners.forEach(listener -> { |
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.
@fudongyingluck Seems like there is a potential behavior change here. The NotifyOnceListener will only take action on the first invocation, and all subsequent invocations are no-ops. You're removing that behavior by using a basic Consumer. How do you know this won't cause a regression?
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 for your time. I think the "all subsequent invocations" means the innerOnFailure(). Although there is a little change here, I do not think we should surround the exception layer after layer. The code is very hard to read. And innerOnFailure() actually has another way to complete it. Since I haven't worked on this part recently, this issue will be closed. Thanks for your time again ~
Catch onResponse exception in onFailure is unnecessarily, and hard to read. The callback is more suitable, I thought