-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Reject multiple data paths on frozen capable nodes #71896
Reject multiple data paths on frozen capable nodes #71896
Conversation
Nodes with a frozen cache no longer supports multiple data paths. This simplifies cache sizing and avoids the need to support multiple cache files. Relates elastic#71844
Pinging @elastic/es-distributed (Team:Distributed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i've left just one comment, o.w. looking good.
final List<String> dataPaths = (List<String>) settings.get(Environment.PATH_DATA_SETTING); | ||
if (dataPaths.size() > 1) { | ||
throw new SettingsException( | ||
"setting [{}={}] is not permitted on nodes with multiple data paths [{}]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this interact with the other PR (default frozen cache size) when you have multiple data paths defined and a dedicated frozen node defined, but have not explicitly configured a cache size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is my intention to make that case fail once I merge the two together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
} | ||
} | ||
} | ||
assertTrue("no shard folder found", shardFolderFound); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe indicate the name of the index in the error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in 3163266
final ShardPath shardPath = ShardPath.loadShardPath(logger, service, shardId, customDataPath); | ||
if (shardPath != null && Files.exists(shardPath.getDataPath())) { | ||
shardFolderFound = true; | ||
assertEquals(snapshotDirectory, Files.notExists(shardPath.resolveIndex())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we could determine snapshotDirectory
from the index settings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I simply moved this method from SearchableSnapshotsIntegTests
so will defer improving this to a follow-up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…ct_multiple_data_paths
…ct_multiple_data_paths
Nodes with a frozen cache no longer supports multiple data paths. This simplifies cache sizing and avoids the need to support multiple cache files. Relates elastic#71844
PartiallyCachedShardAllocationIntegTests does not support multiple data paths, since it uses frozen cache, fixed. Caused by elastic#71896
PartiallyCachedShardAllocationIntegTests does not support multiple data paths, since it uses frozen cache, fixed. Caused by #71896
Nodes with a frozen cache no longer supports multiple data paths. This
simplifies cache sizing and avoids the need to support multiple cache
files.
Relates #71844 (which also contains the single line documentation on this).