Skip to content
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

TEST: getCapturedRequestsAndClear should be atomic #31312

Merged
merged 3 commits into from
Jun 15, 2018

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Jun 14, 2018

We might lose messages between getCapturedRequestsAndClear calls. This
commit makes sure that both getCapturedRequestsAndClear and
getCapturedRequestsByTargetNodeAndClear are atomic.

We might lose messages between getCapturedRequestsAndClear calls. This
commit makes sure that both getCapturedRequestsAndClear and
getCapturedRequestsByTargetNodeAndClear are atomic.
@dnhatn dnhatn added >non-issue >test Issues or PRs that are addressing/adding tests :Distributed/Network Http and internode communication implementations v7.0.0 v6.4.0 labels Jun 14, 2018
@dnhatn dnhatn requested a review from jasontedor June 14, 2018 00:52
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I left one suggestion.

private Map<String, List<CapturedRequest>> groupRequestsByTargetNode(Collection<CapturedRequest> requests) {
Map<String, List<CapturedRequest>> result = new HashMap<>();
for (CapturedRequest request : requests) {
result.compute(request.node.getId(), (k, group) -> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is simpler?

diff --git a/test/framework/src/main/java/org/elasticsearch/test/transport/CapturingTransport.java b/test/framework/src/main/java/org/elasticsearch/test/transport/CapturingTransport.java
index a7a0eb446e8..c8f87e75e15 100644
--- a/test/framework/src/main/java/org/elasticsearch/test/transport/CapturingTransport.java
+++ b/test/framework/src/main/java/org/elasticsearch/test/transport/CapturingTransport.java
@@ -103,13 +103,7 @@ public class CapturingTransport implements Transport {
     private Map<String, List<CapturedRequest>> groupRequestsByTargetNode(Collection<CapturedRequest> requests) {
         Map<String, List<CapturedRequest>> result = new HashMap<>();
         for (CapturedRequest request : requests) {
-            result.compute(request.node.getId(), (k, group) -> {
-                if (group == null) {
-                    group = new ArrayList<>();
-                }
-                group.add(request);
-                return group;
-            });
+            result.computeIfAbsent(request.node.getId(), k -> new ArrayList<>()).add(request);
         }
         return result;
     }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@dnhatn
Copy link
Member Author

dnhatn commented Jun 15, 2018

Thanks @jasontedor

@dnhatn dnhatn merged commit e5b7137 into elastic:master Jun 15, 2018
@dnhatn dnhatn deleted the capturing-transport branch June 15, 2018 01:32
dnhatn added a commit that referenced this pull request Jun 15, 2018
We might lose messages between getCapturedRequestsAndClear calls.
This commit makes sure that both getCapturedRequestsAndClear and
getCapturedRequestsByTargetNodeAndClear are atomic.
tlrx added a commit that referenced this pull request Jun 15, 2018
* master:
  992c788 Uncouple persistent task state and status (#31031)
  8c6ee7d Describe how to add a plugin in Dockerfile (#31340)
  1c5cec0 Remove http status code maps (#31350)
  87a676e Do not set vm.max_map_count when unnecessary (#31285)
  e5b7137 TEST: getCapturedRequestsAndClear should be atomic (#31312)
  0324103 Painless: Fix bug for static method calls on interfaces (#31348)
  d6d0727 QA: Fix resolution of default distribution (#31351)
  fcf1e41 Extract common http logic to server (#31311)
  6dd81ea Build: Fix the license in the pom zip and tar (#31336)
  8f886cd Treat ack timeout more like a publish timeout (#31303)
  9b29327 [ML] Add description to ML filters (#31330)
  f7a0caf SQL: Fix build on Java 10
  375d09c [TEST] Fix RemoteClusterClientTests#testEnsureWeReconnect
  4877cec More detailed tracing when writing metadata (#31319)
  bbfe1ec [Tests] Mutualize fixtures code in BaseHttpFixture (#31210)
tlrx added a commit that referenced this pull request Jun 15, 2018
* 6.x:
  6d711fa (origin/6.x, 6.x) Uncouple persistent task state and status (#31031)
  f0f16b7 [TEST] Make SSL restrictions update atomic (#31050)
  652193f Describe how to add a plugin in Dockerfile (#31340)
  bb568ab TEST: getCapturedRequestsAndClear should be atomic (#31312)
  6f94914 Do not set vm.max_map_count when unnecessary (#31285)
  c12f3c7 Painless: Fix bug for static method calls on interfaces (#31348)
  21128e2 QA: Fix resolution of default distribution (#31351)
  df17a83 Build: Fix the license in the pom zip and tar (#31336)
  3e76b15 Treat ack timeout more like a publish timeout (#31303)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Network Http and internode communication implementations >non-issue >test Issues or PRs that are addressing/adding tests v6.4.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants