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.

Sign up for GitHub

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

Jump to bottom

Reintroduce mandatory http pipelining support #30820

Merged
merged 3 commits into from
May 23, 2018
Merged

Reintroduce mandatory http pipelining support #30820

merged 3 commits into from
May 23, 2018

Conversation

Tim-Brooks
Copy link
Contributor

This commit reintroduces 31251c9 and 63a5799. These commits introduced a
memory leak and were reverted. This commit brings those commits back
and fixes the memory leak by removing unnecessary retain method calls.

Tim-Brooks added 3 commits May 23, 2018 08:49
@Tim-Brooks
Fix memory leak in http pipelining
415c7c0 
This is related to elastic#30801. When we calling the http pipelining
aggregator on an inbound request, we retain the netty request. However,
this is unnecessary as the pipelining aggregator does not store the
request. This worked in the past as we manually release the request and
netty internally automatically releases the request. At this point we do
not implement the ref counter interface after the pipelining step which
means that netty is no longer automatically handling this second retain.

This commit removes that retain.
@Tim-Brooks
Revert "Revert "Make http pipelining support mandatory (elastic#30695)…
f0833eb 
…" (elastic#30813)"

This reverts commit 4fd0a3e.
@Tim-Brooks
Merge branch 'fix_ref_count' into add_http_pipelining_work_back
a49db84
@Tim-Brooks Tim-Brooks added >enhancement review :Distributed Coordination/Network Http and internode communication implementations v7.0.0 labels May 23, 2018
@Tim-Brooks Tim-Brooks requested review from s1monw and jasontedor May 23, 2018 17:09
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@Tim-Brooks Tim-Brooks mentioned this pull request May 23, 2018
Closed
@Tim-Brooks
Copy link
Contributor Author

This is just these three commits combined:

#30695
#30788
#30815

That is: the commit that made pipelining support mandatory, the commit that cleaned up some integration tests, and the commit that should fix the memory leak.

The first two commits were reverted to fix CI for other PRs.

jasontedor
jasontedor approved these changes May 23, 2018
View reviewed changes
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.

@Tim-Brooks Tim-Brooks merged commit d7040ad into elastic:master May 23, 2018
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request May 23, 2018
@jasontedor
Merge branch 'master' into stable-file-modes
f4427d9 
* master:
  [DOCS] Splits auditing.asciidoc into smaller files
  Reintroduce mandatory http pipelining support (elastic#30820)
  Painless: Types Section Clean Up (elastic#30283)
  Add support for indexed shape routing in geo_shape query (elastic#30760)
  [test] java tests for archive packaging (elastic#30734)
  Revert "Make http pipelining support mandatory (elastic#30695)" (elastic#30813)
  [DOCS] Fix more edit URLs in Stack Overview (elastic#30704)
  Use correct cluster state version for node fault detection (elastic#30810)
  Change serialization version of doc-value fields.
  [DOCS] Fixes broken link for native realm
  [DOCS] Clarified audit.index.client.hosts (elastic#30797)
  [TEST] Don't expect acks when isolating nodes
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request May 23, 2018
@jasontedor
Merge branch 'master' into verify-official-plugin-signature
ea11c1a 
* master: (25 commits)
  [DOCS] Splits auditing.asciidoc into smaller files
  Reintroduce mandatory http pipelining support (elastic#30820)
  Painless: Types Section Clean Up (elastic#30283)
  Add support for indexed shape routing in geo_shape query (elastic#30760)
  [test] java tests for archive packaging (elastic#30734)
  Revert "Make http pipelining support mandatory (elastic#30695)" (elastic#30813)
  [DOCS] Fix more edit URLs in Stack Overview (elastic#30704)
  Use correct cluster state version for node fault detection (elastic#30810)
  Change serialization version of doc-value fields.
  [DOCS] Fixes broken link for native realm
  [DOCS] Clarified audit.index.client.hosts (elastic#30797)
  [TEST] Don't expect acks when isolating nodes
  Add a `format` option to `docvalue_fields`. (elastic#29639)
  Fixes UpdateSettingsRequestStreamableTests mutate bug
  Mustes {p0=snapshot.get_repository/10_basic/*} YAML test
  Revert "Mutes MachineLearningTests.testNoAttributes_givenSameAndMlEnabled"
  Only allow x-pack metadata if all nodes are ready (elastic#30743)
  Mutes MachineLearningTests.testNoAttributes_givenSameAndMlEnabled
  Use original settings on full-cluster restart (elastic#30780)
  Only ack cluster state updates successfully applied on all nodes (elastic#30672)
  ...
dnhatn added a commit that referenced this pull request May 24, 2018
@dnhatn
Merge branch 'master' into ccr
3fb2c45 
* master:
  [DOCS] Fixes typos in security settings
  Fix GeoShapeQueryBuilder serialization after backport
  [DOCS] Splits auditing.asciidoc into smaller files
  Reintroduce mandatory http pipelining support (#30820)
  Painless: Types Section Clean Up (#30283)
  Add support for indexed shape routing in geo_shape query (#30760)
  [test] java tests for archive packaging (#30734)
  Revert "Make http pipelining support mandatory (#30695)" (#30813)
  [DOCS] Fix more edit URLs in Stack Overview (#30704)
  Use correct cluster state version for node fault detection (#30810)
  Change serialization version of doc-value fields.
  [DOCS] Fixes broken link for native realm
  [DOCS] Clarified audit.index.client.hosts (#30797)
  [TEST] Don't expect acks when isolating nodes
  Add a `format` option to `docvalue_fields`. (#29639)
  Fixes UpdateSettingsRequestStreamableTests mutate bug
  Mustes {p0=snapshot.get_repository/10_basic/*} YAML test
  Revert "Mutes MachineLearningTests.testNoAttributes_givenSameAndMlEnabled"
  Only allow x-pack metadata if all nodes are ready (#30743)
  Mutes MachineLearningTests.testNoAttributes_givenSameAndMlEnabled
  Use original settings on full-cluster restart (#30780)
  Only ack cluster state updates successfully applied on all nodes (#30672)
  Expose Lucene's FeatureField. (#30618)
  Fix a grammatical error in the 'search types' documentation.
  Remove http pipelining from integration test case (#30788)
martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request May 25, 2018
@martijnvg
Merge remote-tracking branch 'es/ccr' into ccr_create_and_follow
1687020 
* es/ccr: (55 commits)
  [DOCS] Fixes typos in security settings
  Fix GeoShapeQueryBuilder serialization after backport
  [DOCS] Splits auditing.asciidoc into smaller files
  Reintroduce mandatory http pipelining support (elastic#30820)
  Painless: Types Section Clean Up (elastic#30283)
  Add support for indexed shape routing in geo_shape query (elastic#30760)
  [test] java tests for archive packaging (elastic#30734)
  Revert "Make http pipelining support mandatory (elastic#30695)" (elastic#30813)
  [DOCS] Fix more edit URLs in Stack Overview (elastic#30704)
  Use correct cluster state version for node fault detection (elastic#30810)
  Change serialization version of doc-value fields.
  [DOCS] Fixes broken link for native realm
  [DOCS] Clarified audit.index.client.hosts (elastic#30797)
  [TEST] Don't expect acks when isolating nodes
  Mute CorruptedFileIT in CCR
  Add a `format` option to `docvalue_fields`. (elastic#29639)
  Fixes UpdateSettingsRequestStreamableTests mutate bug
  Mustes {p0=snapshot.get_repository/10_basic/*} YAML test
  Revert "Mutes MachineLearningTests.testNoAttributes_givenSameAndMlEnabled"
  Only allow x-pack metadata if all nodes are ready (elastic#30743)
  ...
@jasontedor jasontedor mentioned this pull request May 25, 2018
Closed
@danielmitterdorfer danielmitterdorfer mentioned this pull request May 31, 2018
Closed
This was referenced Jul 25, 2018
Merged
Closed
@Tim-Brooks Tim-Brooks deleted the add_http_pipelining_work_back branch December 10, 2018 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasontedor jasontedor jasontedor approved these changes

@s1monw s1monw Awaiting requested review from s1monw

Assignees
No one assigned
Labels
:Distributed Coordination/Network Http and internode communication implementations >enhancement v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Tim-Brooks @elasticmachine @jasontedor @colings86