Simplify BlobStoreRepository (Flatten Nested Classes) #42833
Conversation
* In the current codebase it is hardly obvious what code operates on a shard and is run by a datanode what code operates on the global metadata and is run on master * Fixed by adjusting the method names accordingly * The nested context classes don't add much if any value, they simply spread out the parameters that go into a shard snapshot create or delete all over the place since their constructors can be inlined in all spots * Fixed by flattening the nested classes into BlobStoreRepository * Also: * Inlined the other single use inner classes * Moved all methods related to shard directories together to the bottom of the class * Dried up the logic for getting index and shard blob containers
original-brownbear
added
>non-issue
:Distributed Coordination/Snapshot/Restore
|
Pinging @elastic/es-distributed |
There was a problem hiding this comment.
I find it difficult to review this PR, because diffs do not work correctly. Could you please comment on each method of the removed class, what is the replacement method for the removed method and if there are any changes made to it?
Also, probably it makes sense to split this PR into separate PRs. For example, one that just removes nested classes, but makes no other modifications. Another one makes other changes. The splitting might probably produce more readable diffs, at least for the second PR.
| } | ||
| bytes = bStream.bytes(); | ||
| } | ||
| final XContentBuilder builder = XContentFactory.contentBuilder(XContentType.JSON); |
There was a problem hiding this comment.
Inline builder as it's done with repositoryData.snapshotsToXContent?
There was a problem hiding this comment.
Can't do it here since the API is a little different here :( the call to org.elasticsearch.repositories.RepositoryData#incompatibleSnapshotsToXContent returns void.
There was a problem hiding this comment.
I adjusted this now after all to make the API a little more fluent :)
| @@ -796,48 +785,6 @@ private void writeAtomic(final String blobName, final BytesReference bytesRef, b | |||
| } | |||
| } | |||
|
|
|||
| @Override | |||
| public void snapshotShard(Store store, MapperService mapperService, SnapshotId snapshotId, IndexId indexId, | |||
There was a problem hiding this comment.
Git diff is failing here because seems that this method and the next one are not removed.
There was a problem hiding this comment.
I made this (hopefully) a lot more agreeable to look at now but removing the shuffling of methods.
Try diffing with whitespaces ignored #42833 maybe, then all but this method should come up fine.
In this method nothing really changed except for the fact that I moved to using the cached time thread and moved the counter variables a little closer to usage. Other than that, the method here is equivalent to the snapshot method on the context before (except that the fields on the context are now method parameters).
Hope that helps :)
|
Jenkins run elasticsearch-ci/oss-distro-docs |
|
@tlrx sure on it, I'll separate out the smaller pieces and leave the flattening of the class structure in this one. |
* Extracted from elastic#42833: * Dry up index and shard path handling * Shorten XContent handling
|
Small and unrelated things extracted in #43323 :) |
|
Jenkins run elasticsearch-ci/1 |
* Some Cleanup in BlobStoreRepository * Extracted from #42833: * Dry up index and shard path handling * Shorten XContent handling
|
@tlrx alrighty, merged in |
original-brownbear
changed the title
|
Thanks so much for going through this one @tlrx! All points addressed/fixed I think :) |
|
Thank you @tlrx ! |
* Some Cleanup in BlobStoreRepository * Extracted from elastic#42833: * Dry up index and shard path handling * Shorten XContent handling
* In the current codebase it is hardly obvious what code operates on a shard and is run by a datanode what code operates on the global metadata and is run on master * Fixed by adjusting the method names accordingly * The nested context classes don't add much if any value, they simply spread out the parameters that go into a shard snapshot create or delete all over the place since their constructors can be inlined in all spots * Fixed by flattening the nested classes into BlobStoreRepository * Also: * Inlined the other single use inner classes
* In the current codebase it is hardly obvious what code operates on a shard and is run by a datanode what code operates on the global metadata and is run on master * Fixed by adjusting the method names accordingly * The nested context classes don't add much if any value, they simply spread out the parameters that go into a shard snapshot create or delete all over the place since their constructors can be inlined in all spots * Fixed by flattening the nested classes into BlobStoreRepository * Also: * Inlined the other single use inner classes
constructors can be inlined in all spots