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

System properties and configuration settings must not be forwarded to tribe clients #9721

Merged
merged 1 commit into from Mar 7, 2015

Conversation

javanna
Copy link
Member

@javanna javanna commented Feb 17, 2015

The tribe node, at startup, sets up the tribe clients that will join their corresponding tribes. All of the tribe.* settings are properly forwarded to the corresponding tribe client. System properties and global configuration settings (all but tribe.*) must not be forwarded to the tribe clients though or they will end up overriding per tribe settings with same name.

For instance if you set the transport.tcp.port to some defined value for the tribe node, via system property or configuration file, that same value must not be forwarded to the tribe clients, otherwise they will try and use the same port, which will be already occupied by the tribe node itself, resulting in startup failed. Same for cluster.name, which will cause the tribe clients not to join their tribes.

This change is quite hard to test in our test infra, given that we make sure that the internal test cluster ignores any system property for proper test isolation. I tested it manually and verfied that the fix works and doesn't seem to have any downside. If anybody has any idea on how we can test this, I would be happy to add some tests to make sure we have proper coverage for it.

Closes #9576

@s1monw
Copy link
Contributor

s1monw commented Feb 17, 2015

yeah I agree with this. Can we have a simple unittest that fails if we do it still?

@kimchy
Copy link
Member

kimchy commented Feb 17, 2015

@javanna double checking, the way we document to setup tribe nodes works still? I remember running into issues without doing that, can't exactly recall, I am good with the existing documentation around setting it up stands. I would also conceptually mark it as breaking and explain the change.

@javanna
Copy link
Member Author

javanna commented Feb 17, 2015

good point @kimchy to my mind this change shouldn't be a breaking one. All of the tribe.* settings will still be copied from the tribe node the tribe clients, regardless of how they were initially set (e.g. -Des.tribe.t1.cluster.name=tribe1 gets forwarded to the corresponding tribe client for tribe t1 although it was set via sysprop). But global settings (all but tribe.*) that are either in elasticsearch.yml or provided through system property shouldn't be automatically forwarded to all of the tribes. Makes sense? Let me know what you think, I might be missing something here. I will also double check and run some more tests just to make sure.

@kimchy
Copy link
Member

kimchy commented Mar 2, 2015

@javanna I am good with the change, its cleaner I think, just wanted to make sure that the current user experience with setting up tribe node stands (mainly around the docs on how to set it up)

@javanna
Copy link
Member Author

javanna commented Mar 2, 2015

agreed @kimchy I think we are good I am working on tests, will ping you again for a final review

@javanna javanna force-pushed the fix/tribe_node_sysprop_config branch from 29fbb14 to 6636341 Compare March 3, 2015 09:06
@javanna
Copy link
Member Author

javanna commented Mar 3, 2015

I pushed a new commit with a unit test that verifies that sys props and config parameters are not re-read by the tribe client nodes. They instead receive only the needed settings, handed over by the tribe node itself. I don't see any downside of this, but again let me know if you can think of any.

@kimchy
Copy link
Member

kimchy commented Mar 6, 2015

LGTM

@javanna javanna force-pushed the fix/tribe_node_sysprop_config branch from 6636341 to 20e2a65 Compare March 6, 2015 17:45
@javanna javanna force-pushed the fix/tribe_node_sysprop_config branch from 20e2a65 to 8ebcd58 Compare March 6, 2015 18:16
@javanna javanna removed the review label Mar 7, 2015
…forwarded to tribe clients

The tribe node, at startup, sets up the tribe clients that will join their corresponding tribes. All of the tribe.* settings are properly forwarded to the corresponding tribe client. System properties and global configuration settings must not be forwarded to the tribe client though or they will end up overriding per tribe settings with same name causing issues.

 For instance if you set the transport.tcp.port to some defined value for the tribe node, via system property or configuration file, that same value must not be forwarded to the tribe clients, otherwise they will try and use the same port, which will be already occupied by the tribe node itself, resulting in startup failed. Same for cluster.name, which will cause the tribe clients not to join their tribes.

Closes elastic#9576
Closes elastic#9721
@javanna javanna force-pushed the fix/tribe_node_sysprop_config branch from 8ebcd58 to 2d2ba48 Compare March 7, 2015 08:26
javanna added a commit that referenced this pull request Mar 7, 2015
…forwarded to tribe clients

The tribe node, at startup, sets up the tribe clients that will join their corresponding tribes. All of the tribe.* settings are properly forwarded to the corresponding tribe client. System properties and global configuration settings must not be forwarded to the tribe client though or they will end up overriding per tribe settings with same name causing issues.

 For instance if you set the transport.tcp.port to some defined value for the tribe node, via system property or configuration file, that same value must not be forwarded to the tribe clients, otherwise they will try and use the same port, which will be already occupied by the tribe node itself, resulting in startup failed. Same for cluster.name, which will cause the tribe clients not to join their tribes.

Closes #9576
Closes #9721
javanna added a commit that referenced this pull request Mar 7, 2015
…forwarded to tribe clients

The tribe node, at startup, sets up the tribe clients that will join their corresponding tribes. All of the tribe.* settings are properly forwarded to the corresponding tribe client. System properties and global configuration settings must not be forwarded to the tribe client though or they will end up overriding per tribe settings with same name causing issues.

 For instance if you set the transport.tcp.port to some defined value for the tribe node, via system property or configuration file, that same value must not be forwarded to the tribe clients, otherwise they will try and use the same port, which will be already occupied by the tribe node itself, resulting in startup failed. Same for cluster.name, which will cause the tribe clients not to join their tribes.

Closes #9576
Closes #9721
@javanna javanna merged commit 2d2ba48 into elastic:master Mar 7, 2015
@clintongormley clintongormley changed the title Tribe node: system properties and configuration settings must not be forwarded to tribe clients System properties and configuration settings must not be forwarded to tribe clients Jun 8, 2015
mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015
…forwarded to tribe clients

The tribe node, at startup, sets up the tribe clients that will join their corresponding tribes. All of the tribe.* settings are properly forwarded to the corresponding tribe client. System properties and global configuration settings must not be forwarded to the tribe client though or they will end up overriding per tribe settings with same name causing issues.

 For instance if you set the transport.tcp.port to some defined value for the tribe node, via system property or configuration file, that same value must not be forwarded to the tribe clients, otherwise they will try and use the same port, which will be already occupied by the tribe node itself, resulting in startup failed. Same for cluster.name, which will cause the tribe clients not to join their tribes.

Closes elastic#9576
Closes elastic#9721
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tribe node setup
4 participants