Remove environment from transport client #13383
Conversation
Transport clients run embedded within external applications, so elasticsearch should not be doing anything with the filesystem, as there is not elasticsearch home. This change makes a number of cleanups to the internal API for loading settings and creating an environment. The loadFromConfig option was removed, since it was always true except for tests. We now always attempt to load settings from config a file when an environment is created. The prepare methods were also simplified so there is now prepareSettingsAndEnvironment which nodes use, and prepareSettings which the transport client uses. I also attempted to improve the tests, but there is a still a lot of follow up work to do there. closes elastic#13155
rjernst
added
>breaking
blocker
:Core/Infra/Settings
| @@ -197,7 +197,7 @@ private static void setupLogging(Settings settings, Environment environment) { | |||
|
|
|||
| private static Tuple<Settings, Environment> initialSettings(boolean foreground) { | |||
| Terminal terminal = foreground ? Terminal.DEFAULT : null; | |||
| return InternalSettingsPreparer.prepareSettings(EMPTY_SETTINGS, true, terminal); | |||
| return InternalSettingsPreparer.prepareSettingsAndEnvironment(EMPTY_SETTINGS, terminal); | |||
There was a problem hiding this comment.
it might be simpler to just name this InternalSettingsPreparer.prepareEnvironment and return only Environment since it has the settings already as a member then we can spare the tuple and things are less funky?
There was a problem hiding this comment.
I think the only issue with that right now is the last line of the method, which modifies the log file setting to be absolute, after creating the environment.
There was a problem hiding this comment.
yeah I wonder if we should do this first?
There was a problem hiding this comment.
well it is using the resolved config dir from the environment...
There was a problem hiding this comment.
but i agree, this is already breaking. i will move it and simplify as you suggested
|
left some minors LGTM otherwise |
|
@rjernst could you also add a note to the breaking changes docs please: https://www.elastic.co/guide/en/elasticsearch/reference/2.0/_java_api_changes.html |
|
@clintongormley what is the breaking change here? |
|
@s1monw the fact that the node client now requires a |
settings/environment
|
It's also breaking because the @clintongormley I added a note to the migration guide. |
|
@s1monw I updated to remove the extra path.logs setter at the end of prepare. It was only there for logging, so I moved it directly to the logging configurator. |
|
LGTM |
Remove environment from transport client
Remove environment from transport client
Remove environment from transport client
|
2.x commit: 2d698d6 |
The tribe node creates one local client node for each cluster it connects to. Refactorings in elastic#13383 broke this so that each local client node now tries to load the full elasticsearch.yml that the real tribe node uses. This change fixes the problem by adding a TribeClientNode which is a subclass of Node. The Environment the node uses is now passed in (in place of Settings), and the TribeClientNode simply does not use InternalSettingsPreparer.prepareEnvironment. The tests around tribe nodes are not great. The existing tests pass, but I also manually tested by creating 2 local clusters, and configuring and starting a tribe node. With this I was able to see in the logs the tribe node connecting to each cluster. closes elastic#13383
|
Just ran into this with an upgrade from 1.7 to 2.2, since we were using I want to point out that the documentation (here) is wrong to suggest you can still do this.
|
Remove comment about configuring the transport client in the config file Relates to #13383 (comment)
|
thanks @mtraynham - fixed in 1769e9b |
Remove comment about configuring the transport client in the config file Relates to #13383 (comment)
Remove comment about configuring the transport client in the config file Relates to #13383 (comment)
Transport clients run embedded within external applications, so
elasticsearch should not be doing anything with the filesystem, as there
is not elasticsearch home.
This change makes a number of cleanups to the internal API for loading
settings and creating an environment. The loadFromConfig option was
removed, since it was always true except for tests. We now always
attempt to load settings from config a file when an environment is
created. The prepare methods were also simplified so there is now
prepareSettingsAndEnvironment which nodes use, and prepareSettings which
the transport client uses. I also attempted to improve the tests, but
there is a still a lot of follow up work to do there.
closes #13155
NOTE: I left a handful of TODOs for things that looked fishy in prepareSettingsAndEnvironment. In general this should really be cleaned up to be much simpler. There are far to many ways to configure the settings for a developer to understand the code, so I don't know how a user would understand whether to put a setting in properties, env vars, or one of many possible files, or forcing a property, or setting a default, etc.