Skip to content

Conversation

@talevy
Copy link
Contributor

@talevy talevy commented Oct 29, 2018

ILM's Shrink Action was using a nodes "_name" attribute to
allocate to prepare for the shrink step. Since the name is
configurable by a user and may use the same name for
multiple nodes on one machine, _id is safer since it is guaranteed
to be unique.

closes #35043.

ILM's Shrink Action was using a nodes "_name" attribute to
allocate to prepare for the shrink step. Since the name is
configurable by a user and may use the same name for
multiple nodes on one machine, _id is safer since it is guaranteed
to be unique.

closes elastic#35043.
@talevy talevy added the :Data Management/ILM+SLM Index and Snapshot lifecycle management label Oct 29, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@gwbrown gwbrown left a comment

Choose a reason for hiding this comment

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

Left one question, otherwise LGTM

String nodeName = "node_" + i;
int nodePort = 9300 + i;
Settings nodeSettings = Settings.builder().put(validNodeSettings).put("node.name", nodeName).build();
Settings nodeSettings = Settings.builder().put(validNodeSettings).put(Node.NODE_NAME_SETTING.getKey(), nodeName).build();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need to set the node names here if we're not doing anything with them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, I felt it was arbitrary since the whole goal is to "fake a node" in unit tests anyways, so I decided to leave it unchanged.

@talevy
Copy link
Contributor Author

talevy commented Oct 29, 2018

test this please

Copy link
Contributor

@colings86 colings86 left a comment

Choose a reason for hiding this comment

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

LGTM

@talevy
Copy link
Contributor Author

talevy commented Oct 30, 2018

test this please

@talevy talevy added the blocker label Oct 30, 2018
@talevy
Copy link
Contributor Author

talevy commented Oct 31, 2018

test this please

2 similar comments
@talevy
Copy link
Contributor Author

talevy commented Oct 31, 2018

test this please

@talevy
Copy link
Contributor Author

talevy commented Oct 31, 2018

test this please

@talevy
Copy link
Contributor Author

talevy commented Oct 31, 2018

test this please

@talevy
Copy link
Contributor Author

talevy commented Nov 1, 2018

OK, so I thought these were related to the flaky tests, but something is broken here. will dig in tomorrow. The change does not seem to work

@talevy
Copy link
Contributor Author

talevy commented Nov 1, 2018

test this please

1 similar comment
@talevy
Copy link
Contributor Author

talevy commented Nov 1, 2018

test this please

@talevy talevy merged commit b260406 into elastic:index-lifecycle Nov 1, 2018
@talevy talevy deleted the ilm-shrink-id branch November 1, 2018 21:38
talevy added a commit that referenced this pull request Nov 1, 2018
ILM's Shrink Action was using a nodes "_name" attribute to
allocate to prepare for the shrink step. Since the name is
configurable by a user and may use the same name for
multiple nodes on one machine, _id is safer since it is guaranteed
to be unique.

closes #35043.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blocker :Data Management/ILM+SLM Index and Snapshot lifecycle management

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants