When checking if key exists in ThreadContextStruct:putHeaders() method，should put requestHeaders in map first #26068

hanbj · 2017-08-05T09:15:48Z

1、The parameter headers does not have the same key, so the checksum doesn't make sense
2、When checking that key already exists in requestHeaders, you should first put

elasticmachine · 2017-08-05T09:15:50Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

hanbj · 2017-08-11T14:28:56Z

@cbuescher This PR has been on for a long time and hasn't been noticed. I hope the great God can review it. Thanks

cbuescher · 2017-08-11T14:38:40Z

@hanbj Sorry, but we get to it when we get to it. 6 days isn't really that long when you consider we have over a hundred open PRs, lots of open issues. I hope you understand. In the meantime it might help to understand what problem this change is trying to solve, did you run into a problem with this? Is there an open issue this fixes?

hanbj · 2017-08-11T16:20:30Z

@cbuescher I'm very very sorry. I didn't mean that. I'm sorry to trouble you

cbuescher · 2017-08-11T16:30:51Z

No problem, I just wanted to explain that PRs get picked up whenever anybody has time to review them.
I saw this is a follow up to #26037, but even reading that I don't quiet get the problem this is trying to solve. It probably helps anybody who will eventually review this issue if you try to motivate the chage you propose, e.g. by explaining a bug that this fixes or any other reason you think this change is necessary.

cbuescher · 2017-08-11T16:37:08Z

And I forgot to add, thanks for taking the time and effort to look at this and submit a PR at all ;-)

rjernst · 2017-08-14T18:44:49Z

The change looks correct to me, but I think it needs a test (in ThreadContextTests).

hanbj · 2017-08-15T02:39:21Z

@rjernst I add a test. Thank you.

rjernst

Thanks for the added test. I left a couple minor comments. Once those are addressed I will kick off CI (it would fail right now due to wildcard imports).

rjernst · 2017-08-16T20:46:53Z

core/src/test/java/org/elasticsearch/common/util/concurrent/ThreadContextTests.java

-import static org.hamcrest.Matchers.hasItem;
-import static org.hamcrest.Matchers.hasSize;
-import static org.hamcrest.Matchers.sameInstance;
+import static org.hamcrest.Matchers.*;


Did you run gradle precommit? This will fail. We do not use wildcard imports.

Sorry, I often use formatting shortcut keys.

rjernst · 2017-08-16T20:47:58Z

core/src/test/java/org/elasticsearch/common/util/concurrent/ThreadContextTests.java

+        assertEquals("bar", threadContext.getHeader("foo"));
+        try {
+            threadContext.putHeader(Collections.<String, String>singletonMap("foo", "boom"));
+        } catch (IllegalArgumentException e) {


You should use expectThrows(...) instead of try/catch.

Yes,so much the better.

rjernst · 2017-08-16T20:51:55Z

core/src/main/java/org/elasticsearch/common/util/concurrent/ThreadContext.java

@@ -407,11 +407,10 @@ private ThreadContextStruct putHeaders(Map<String, String> headers) {
            if (headers.isEmpty()) {
                return this;
            } else {
-                final Map<String, String> newHeaders = new HashMap<>();
+                final Map<String, String> newHeaders = new HashMap<>(this.requestHeaders);


I just realized I'm unsure about this fix. The putSingleHeader below will fail if the key already exists, so anything in requestHeaders can then not be overriden. I think what we want instead is a loop over requestHeaders after the headers loop, but which uses putIfAbsent?

I didn't quite catch what you mean.
First of all, I didn't submit this PR before.
1, create a new HashMap instance (newHeaders).
2, traversal parameter headers.
The two step above means check whether the headers's key if exists in newHeaders.
The parameter headers is a Map. It doesn't have the same key. Just put the data in headers into newHeaders.

What we need to do is check if the parameter headers's key already exists in requestHeaders.

If this logic is not changed, the following:
newHeaders.putAll (this.requestHeaders);
requestHeaders also overwrites the same key in headers, but the user doesn't know it.
For instance：
headers: ("foo", "bar")
requestHeaders: ("foo", "boom")
After running newHeaders.putAll (this.requestHeaders);
Will become: newHeaders: ("foo", "boom")
However, the user thinks the value corresponding to the foo is bar

I think you misunderstood my comment. Look at the implementation of putSingleHeader, which is what the loop over headers calls to add each header. It will throw an exception if a key already exists. This means with your change if a key exists in requestHeaders and headers, an exception will be thrown, while before, the value from requestHeaders would have "won". The bug here is that the requestHeaders value should be the fallback, right? So then, instead of initializing the newHeaders to requestHeaders, you can iterate over requestHeaders in place of the current putAll call, and use newHeaders.putIfAbsent, so that the value would only be added if headers did not contain that key.

Alternatively, you can still initialize the newHeaders to requestHeaders, but change the call to putSingleHeader to newHeaders.put. Since headers and requestHeaders are already maps, there is no possibility for duplicate keys within themselves.

I know what you mean now. You don't want to thrown an exception.
I found it throw an exception in the putRequest method, So I did the same.
I've also found that in the stashAndMergeHeaders method, if a key exists in context.requestHeaders and headers, the value from requestHeaders would have "won".
I'm not sure now. I hope you can give me a conclusion. if a key exists in requestHeaders and headers, whether requestHeaders to overwrite headers or headers to overwrite requestHeaders.
Thank you.

Since the method takes in headers, I assume those should "win" over what was in the context headers map before. However, I did notice that in stashAndMergeHeaders, the requestHeaders are merged into the headers being passed into putHeaders, and the order is again such that requestHeaders wins over headers. @s1monw can you clarify what the original intention was here?

@rjernst I think the fix is good. it's really just a shortcut for a single header. I wonder if we should de-optimize and just iterate over the map and use putHeader(string,string) instead? also the test looks great

s1monw · 2017-09-25T11:44:33Z

@rjernst do you want to pull this in?

rjernst · 2017-10-06T23:23:37Z

@elasticmachine ok to test

rjernst · 2017-10-09T21:41:32Z

Thanks @hanbj. Sorry for the long delay in merging.

Previously collisions in headers between old and new contexts could be silently ignored, allowing the original context's headers to "win". This commit fixes the headers to require they are disjoint.

* master: (22 commits) Allow only a fixed-size receive predictor (elastic#26165) Add Homebrew instructions to getting started ingest: Fix bug that prevent date_index_name processor from accepting timestamps specified as a json number Scripting: Fix expressions to temporarily support filter scripts (elastic#26824) Docs: Add note to contributing docs warning against tool based refactoring (elastic#26936) Fix thread context handling of headers overriding (elastic#26068) SearchWhileCreatingIndexIT: remove usage of _only_nodes update Lucene version for 6.0-RC2 version Calculate and cache result when advanceExact is called (elastic#26920) Test query builder bwc against previous supported versions instead of just the current version. Set minimum_master_nodes on rolling-upgrade test (elastic#26911) Return List instead of an array from settings (elastic#26903) remove _primary and _replica shard preferences (elastic#26791) fixing typo in datehistogram-aggregation.asciidoc (elastic#26924) [API] Added the `terminate_after` parameter to the REST spec for "Count" API Setup debug logging for qa.full-cluster-restart Enable BWC testing against other remotes Use LF line endings in Painless generated files (elastic#26822) [DOCS] Added info about snapshotting your data before an upgrade. Add documentation about disabling `_field_names`. (elastic#26813) ...

* master: Fix handling of paths containing parentheses Allow only a fixed-size receive predictor (elastic#26165) Add Homebrew instructions to getting started ingest: Fix bug that prevent date_index_name processor from accepting timestamps specified as a json number Scripting: Fix expressions to temporarily support filter scripts (elastic#26824) Docs: Add note to contributing docs warning against tool based refactoring (elastic#26936) Fix thread context handling of headers overriding (elastic#26068) SearchWhileCreatingIndexIT: remove usage of _only_nodes

elasticmachine · 2019-01-29T12:34:38Z

Pinging @elastic/es-distributed

bug : ThreadContext----->ThreadContextStruct:putHeaders()

c6b12f7

hanbj mentioned this pull request Aug 5, 2017

When putHeaders, Order problem. #26037

Closed

hanbj changed the title ~~When ThreadContextStruct:putHeaders()，should put requestHeaders in map first~~ When checking if key exists in ThreadContextStruct:putHeaders() method，should put requestHeaders in map first Aug 5, 2017

hanbj mentioned this pull request Aug 7, 2017

Calculate once the number of machine cores #26070

Closed

hanbj added 2 commits August 15, 2017 10:32

test putHeaders

2d0eaa7

delete println

eaba577

rjernst requested changes Aug 16, 2017

View reviewed changes

rjernst reviewed Aug 16, 2017

View reviewed changes

update wildcard imports and instead of try/catch.

b57ecbc

rjernst added 2 commits October 9, 2017 08:12

Merge branch 'master' into review/26068

f54fd7b

Fix line length

09e46a6

rjernst added v6.1.0 v7.0.0 :Internal >bug labels Oct 9, 2017

rjernst merged commit 3ab27d1 into elastic:master Oct 9, 2017

hanbj deleted the threadcontext branch January 16, 2019 06:36

jpountz added the :Distributed Coordination/Network Http and internode communication implementations label Jan 29, 2019

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

When checking if key exists in ThreadContextStruct:putHeaders() method，should put requestHeaders in map first #26068

When checking if key exists in ThreadContextStruct:putHeaders() method，should put requestHeaders in map first #26068

hanbj commented Aug 5, 2017

elasticmachine commented Aug 5, 2017

hanbj commented Aug 11, 2017

cbuescher commented Aug 11, 2017

hanbj commented Aug 11, 2017

cbuescher commented Aug 11, 2017

cbuescher commented Aug 11, 2017

rjernst commented Aug 14, 2017

hanbj commented Aug 15, 2017

rjernst left a comment

rjernst Aug 16, 2017

hanbj Aug 17, 2017

rjernst Aug 16, 2017

hanbj Aug 17, 2017

rjernst Aug 16, 2017 •

edited

Loading

hanbj Aug 17, 2017 •

edited

Loading

hanbj Aug 17, 2017

rjernst Aug 17, 2017 •

edited

Loading

rjernst Aug 17, 2017

hanbj Aug 18, 2017

rjernst Aug 22, 2017

s1monw Sep 18, 2017

s1monw commented Sep 25, 2017

rjernst commented Oct 6, 2017

rjernst commented Oct 9, 2017

elasticmachine commented Jan 29, 2019

When checking if key exists in ThreadContextStruct:putHeaders() method，should put requestHeaders in map first #26068

When checking if key exists in ThreadContextStruct:putHeaders() method，should put requestHeaders in map first #26068

Conversation

hanbj commented Aug 5, 2017

elasticmachine commented Aug 5, 2017

hanbj commented Aug 11, 2017

cbuescher commented Aug 11, 2017

hanbj commented Aug 11, 2017

cbuescher commented Aug 11, 2017

cbuescher commented Aug 11, 2017

rjernst commented Aug 14, 2017

hanbj commented Aug 15, 2017

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjernst Aug 16, 2017 • edited Loading

Choose a reason for hiding this comment

hanbj Aug 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjernst Aug 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s1monw commented Sep 25, 2017

rjernst commented Oct 6, 2017

rjernst commented Oct 9, 2017

elasticmachine commented Jan 29, 2019

rjernst Aug 16, 2017 •

edited

Loading

hanbj Aug 17, 2017 •

edited

Loading

rjernst Aug 17, 2017 •

edited

Loading