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

Detach Transport from TransportService #31727

Merged
merged 5 commits into from
Jul 4, 2018

Conversation

s1monw
Copy link
Contributor

@s1monw s1monw commented Jul 2, 2018

Today TransportService is tightly coupled with Transport since it
requires an instance of TransportService in order to receive reponses
and send requests. This is mainly due to the Request and Reponse handlers
being maintained in TransportService but also because of the lack of a propper
callback interface.

This change moves request handler registry and repsonse handler registration into
Transport and adds all necessary methods to TransportConnectionListener in order
to remove the TransportService dependency from Transport
Transport now accepts one or more TransportConnectionListener instances that are
executed sequentially in a blocking fashion.

Today TransportService is tightly coupled with Transport since it
requires an instance of TransportService in order to receive reponses
and send requests. This is mainly due to the Request and Reponse handlers
being maintained in TransportService but also because of the lack of a propper
callback interface.

This change moves request handler registry and repsonse handler registration into
Transport and adds all necessary methods to `TransportConnectionListener` in order
to remove the `TransportService` dependency from `Transport`
Transport now accepts one or more `TransportConnectionListener` instances that are
executed sequentially in a blocking fashion.
@s1monw s1monw added :Distributed/Network Http and internode communication implementations v7.0.0 >refactoring v6.4.0 labels Jul 2, 2018
@s1monw s1monw requested review from Tim-Brooks and bleskes July 2, 2018 11:49
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

Copy link
Contributor

@Tim-Brooks Tim-Brooks 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 feel like this seems like a reasonable step.

@@ -285,16 +285,47 @@ public void close() {}
return Collections.emptyList();
}

@Override
public long newRequestId() {
return requestId.incrementAndGet();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think with this removal the requestId is now no longer used and can be removed.

* A listener interface that allows to react on transport events. All methods may be
* executed on network threads. Consumers must fork in the case of long running or blocking
* operations.
*/
public interface TransportConnectionListener {

Copy link
Contributor

Choose a reason for hiding this comment

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

A lot of unnecessary semicolons at the end of default methods in this class.

@s1monw s1monw merged commit 3f2a241 into elastic:master Jul 4, 2018
dnhatn added a commit that referenced this pull request Jul 4, 2018
* master:
  [ML] Rate limit established model memory updates (#31768)
  [Docs] Correct default window_size (#31582)
  S3 fixture should report 404 on unknown bucket (#31782)
  Detach Transport from TransportService (#31727)
  [ML] Limit ML filter items to 10K (#31731)
  [ML] Return statistics about forecasts as part of the jobsstats and usage API (#31647)
  Fixture for Minio testing (#31688)
  [DOCS] Add missing get mappings docs to HLRC (#31765)
  [DOCS] Starting Elasticsearch (#31701)
  Painless: Complete Removal of Painless Type (#31699)
  Fix not waiting for Netty ThreadDeathWatcher in IT (#31758)
  Consolidate watcher setting update registration (#31762)
  Build: re-enabled bwc (#31769)
  ingest: Introduction of a bytes processor (#31733)
  Fix coerce validation_method in GeoBoundingBoxQueryBuilder (#31747)
  Add analyze API to high-level rest client (#31577)
  [DOCS] Typos
  DOC: Add examples to the SQL docs (#31633)
  Add support for AWS session tokens (#30414)
  Watcher: Reenable start/stop yaml tests (#31754)
  Implemented XContent serialisation for GetIndexResponse (#31675)
  JDBC: Fix stackoverflow on getObject and timestamp conversion (#31735)
  resolveHasher defaults to NOOP (#31723)
  Account for XContent overhead in in-flight breaker
  Split CircuitBreaker-related tests (#31659)
  Add write*Blob option to replace existing blob (#31729)
  Painless: Add Context Docs (#31190)
  Watcher: Fix chain input toXcontent serialization (#31721)
  Docs: Match the examples in the description (#31710)
  rest-high-level: added get cluster settings (#31706)
  [Docs] Correct typos (#31720)
  Clean up double semicolon code typos (#31687)
  [DOCS] Check for Windows and *nix file paths (#31648)
  [ML] Validate ML filter_id (#31535)
  Revert long lines
  Fix TransportChangePasswordActionTests
s1monw added a commit to s1monw/elasticsearch that referenced this pull request Jul 5, 2018
Today TransportService is tightly coupled with Transport since it
requires an instance of TransportService in order to receive responses
and send requests. This is mainly due to the Request and Response handlers
being maintained in TransportService but also because of the lack of a proper
callback interface.

This change moves request handler registry and response handler registration into
Transport and adds all necessary methods to `TransportConnectionListener` in order
to remove the `TransportService` dependency from `Transport`
Transport now accepts one or more `TransportConnectionListener` instances that are
executed sequentially in a blocking fashion.
s1monw added a commit that referenced this pull request Jul 5, 2018
Today TransportService is tightly coupled with Transport since it
requires an instance of TransportService in order to receive responses
and send requests. This is mainly due to the Request and Response handlers
being maintained in TransportService but also because of the lack of a proper
callback interface.

This change moves request handler registry and response handler registration into
Transport and adds all necessary methods to `TransportConnectionListener` in order
to remove the `TransportService` dependency from `Transport`
Transport now accepts one or more `TransportConnectionListener` instances that are
executed sequentially in a blocking fashion.
dnhatn added a commit that referenced this pull request Jul 5, 2018
* 6.x:
  Test: Do not remove xpack templates when cleaning (#31642)
  SQL: Allow long literals (#31777)
  SQL: Fix incorrect message for aliases (#31792)
  Detach Transport from TransportService (#31727)
  6.3.1 release notes (#31829)
  Add unreleased version 6.3.2
  [ML][TEST] Use java 11 valid time format in DataDescriptionTests (#31817)
  [ML] Don't treat stale FAILED jobs as OPENING in job allocation (#31800)
  [ML] Fix calendar and filter updates from non-master nodes (#31804)
  Fix license header generation on Windows (#31790)
  mark XPackRestIT.test {p0=monitoring/bulk/10_basic/Bulk indexing of monitoring data} as AwaitsFix
  Add JDK11 support without enabling in CI (#31644)
  Watcher: Fix check for currently executed watches (#31137)
  [DOCS] Fixes 6.3.0 release notes (#31771)
  Watcher: Ensure correct method is used to read secure settings (#31753)
  [ML] Rate limit established model memory updates (#31768)
  SQL: Update CLI logo
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 >refactoring v6.4.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants