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

CCS: Drop http address from remote cluster info #29568

Merged
merged 5 commits into from
Apr 27, 2018

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Apr 17, 2018

They are expensive to fetch and no longer needed by Kibana so they
shouldn't be needed by anyone else either.

Closes #29207

They are expensive to fetch and no longer needed by Kibana so they
*shouldn't* be needed by anyone else either.

Closes elastic#29207
@nik9000 nik9000 added >breaking review :Core/Infra/Core Core issues without another label v7.0.0 labels Apr 17, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

connection.getConnectionInfo(actionListener);
}
}
public Stream<RemoteConnectionInfo> getRemoteConnectionInfos() {
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm returning a Stream here because I read https://stackoverflow.com/a/24679745/233833 yesterday. The caller always wants a List but since we have a stream here it seems sane to return the stream and let the caller do what they want with it.

Copy link
Member

Choose a reason for hiding this comment

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

question: what difference does it make given that the only caller calls toList on this Stream all the time?

Copy link
Member Author

Choose a reason for hiding this comment

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

None. I wouldn't go out of my way to return a Stream if I didn't already have one. I just figured, since I do, I may as well return it.

connectionsPerCluster = input.readVInt();
initialConnectionTimeout = input.readTimeValue();
numNodesConnected = input.readVInt();
clusterAlias = input.readString();
if (input.getVersion().onOrAfter(Version.V_6_1_0)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I dropped this because I'm not backporting this change to 6.x because it is breaking so I figured this was a good time to remove the bwc for 6.0.x


@Override
public void writeTo(StreamOutput out) throws IOException {
out.writeList(seedNodes);
Copy link
Member Author

Choose a reason for hiding this comment

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

I moved this next to the ctor because I like to be able to read the read and write on a single screen if possible.

assertNotNull(remoteConnectionInfo);
assertEquals(0, remoteConnectionInfo.numNodesConnected);
assertEquals(0, remoteConnectionInfo.seedNodes.size());
assertEquals(0, remoteConnectionInfo.httpAddresses.size());
Copy link
Member Author

Choose a reason for hiding this comment

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

We don't have the "give up quickly because we're not connected" behavior any more so we return the list of seed nodes.

@javanna javanna self-requested a review April 17, 2018 20:07
@nik9000
Copy link
Member Author

nik9000 commented Apr 19, 2018

@elasticmachine, retest this please.

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

left a question. LGTM though. thanks @nik9000 !

@nik9000
Copy link
Member Author

nik9000 commented Apr 26, 2018

The test failure here is quite real and it is something I'm going to track down.

@nik9000 nik9000 merged commit f4ed902 into elastic:master Apr 27, 2018
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Apr 27, 2018
…st-variables

* origin/master:
  Test: Switch painless test to 1 shard
  CCS: Drop http address from remote cluster info (elastic#29568)
  Reindex: Fold "from old" tests into reindex module (elastic#30142)
  Convert FieldCapabilitiesResponse to a ToXContentObject. (elastic#30182)
  [DOCS] Added 'on a single shard' to description of max_thread_count. Closes 28518 (elastic#29686)
  [TEST] Redirect links to new locations (elastic#30179)
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Apr 27, 2018
* master: (7173 commits)
  Bump changelog version to 6.4 (elastic#30217)
  [DOCS] Adds native realm security settings (elastic#30186)
  Test: Switch painless test to 1 shard
  CCS: Drop http address from remote cluster info (elastic#29568)
  Reindex: Fold "from old" tests into reindex module (elastic#30142)
  Convert FieldCapabilitiesResponse to a ToXContentObject. (elastic#30182)
  [DOCS] Added 'on a single shard' to description of max_thread_count. Closes 28518 (elastic#29686)
  [TEST] Redirect links to new locations (elastic#30179)
  Move repository-s3 fixture tests to QA test project (elastic#29372)
  Fail snapshot operations early on repository corruption (elastic#30140)
  Docs: Document `failures` on reindex and friends
  Build global ordinals terms bucket from matching ordinals (elastic#30166)
  Watcher: Ensure mail message ids are unique per watch action (elastic#30112)
  REST: Remove GET support for clear cache indices (elastic#29525)
  SQL: Correct error message (elastic#30138)
  Require acknowledgement to start_trial license (elastic#30135)
  Fix a bug in FieldCapabilitiesRequest#equals and hashCode. (elastic#30181)
  SQL: Add BinaryMathProcessor to named writeables list (elastic#30127)
  Tests: Use buildDir as base for generated-resources (elastic#30191)
  Fix SliceBuilderTests#testRandom failures
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking :Core/Infra/Core Core issues without another label v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants