Convert path.data to String setting instead of List #72282
Conversation
Since multiple data path support has been removed, the Setting no longer needs to support multiple values. This commit converts the PATH_DATA_SETTING to a String setting from List<String>. relates elastic#71205
rjernst
added
:Core/Infra/Core
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
| @@ -50,7 +50,7 @@ public void setup() throws IOException { | |||
| LogConfigurator.setNodeName("test"); | |||
| Settings settings = Settings.builder() | |||
| .put(Environment.PATH_HOME_SETTING.getKey(), path) | |||
| .putList(Environment.PATH_DATA_SETTING.getKey(), paths) | |||
| .put(Environment.PATH_DATA_SETTING.getKey(), path.resolve("data")) | |||
| @@ -179,7 +170,7 @@ public Settings settings() { | |||
| * The data location. | |||
| */ | |||
| public Path[] dataFiles() { | |||
| return dataFiles; | |||
| return new Path[] { dataFile }; | |||
There was a problem hiding this comment.
Could we (naively) inline this method right now, or in an imminent follow-up? That would leave us with a bunch of places where we do things like this:
for (Path dataPath : new Path[]{environment.dataFile()}) {
I think this is preferable to using environment.dataFiles()[0] to get hold of the unique data path using the knowledge that it's always a singleton array. Unrolling loops like this is obviously correct (indeed IntelliJ will do it for you).
There was a problem hiding this comment.
Yeah, we discussed the approach here to not try to do MDP legacy code removal all at once, keeping each change relatively self-contained, and this is one we agreed to hold for a follow-up.
There was a problem hiding this comment.
Acked. An imminent follow-up, or do we expect to merge things like #72278 first? We call this method in <30 places, so it's a pretty small change that IMO makes the subsequent larger cleanups much neater.
There was a problem hiding this comment.
Yeah, after this change, although we didn’t discuss a dependency on #72278. You’re making a compelling case there should be. Thank you!
…72282)" This reverts commit d933ecd. The revert had two conflicts. The first was very minor in JoinHelper. The second was several tests in PersistedClusterStateServiceTests. relates elastic#78525 relates elastic#71205
Since multiple data path support has been removed, the Setting no longer
needs to support multiple values. This commit converts the
PATH_DATA_SETTING to a String setting from List.
relates #71205