Batching of snapshot state updates #10295
Conversation
javanna
added
:Distributed Coordination/Snapshot/Restore
|
@ywelsch-t have you signed CLA? I cannot find your name there for some reason. |
ywelsch-t
force-pushed
the
batchedsnapshot
branch
from
e5098c8 to
58cdf42
Compare
|
@imotov I'm clearing things with our legal department. Will probably take a week. Sorry for that. |
|
@ywelsch-t any news? |
|
The CLA has been signed last Friday. |
| @@ -459,40 +464,73 @@ public RestoreInfo getRestoreInfo() { | |||
| * @param request update shard status request | |||
| */ | |||
| private void innerUpdateRestoreState(final UpdateIndexShardRestoreStatusRequest request) { | |||
| logger.debug("received updated snapshot restore state [{}]", request); | |||
There was a problem hiding this comment.
This will be logged for every single shard. So, I think it might be better to make it TRACE.
|
@ywelsch-t Indeed the CLA was signed. Thanks a lot for PR. I have reviewed it and left a few comments. Could you also add a test that would verify that shard updates are indeed batching? |
ywelsch-t
force-pushed
the
batchedsnapshot
branch
from
58cdf42 to
692f610
Compare
|
@imotov Thanks for the feedback. I have incorporated all your suggestions. I do not know however how to test that batching indeed occurs (only validated it manually). I looked at the batching of "shard started" actions (which inspired this PR), but could not find a test case to draw inspiration from. |
| } | ||
| assert !batchedRestoreInfo.containsKey(entry.snapshotId()); | ||
| batchedRestoreInfo.put(entry.snapshotId(), | ||
| new Tuple<RestoreInfo, Map<ShardId, ShardRestoreStatus>>( |
There was a problem hiding this comment.
This can be just new Tuple<>(
|
@ywelsch-t I think we are getting there. I left a few minor comments. I think for the test we can do something like this: https://gist.github.com/imotov/6460fe90ffd3678573d7 What do you think? |
|
Test looks good to me (thanks for the help!). If all is good, I can squash the commits and add a proper commit message. |
|
@ywelsch-t great, please also rebase against current master after squashing. |
ywelsch-t
force-pushed
the
batchedsnapshot
branch
from
758a94a to
7af4f9e
Compare
|
@imotov done |
|
@ywelsch-t thanks! I was about to push it to the master, but then I noticed that the test fails from time to time. I will try to figure out and fix it tomorrow and then push. |
Similar to the batching of "shards-started" actions, this commit implements batching of snapshot status updates. This is useful when backing up many indices as the cluster state does not need to be republished as many times. Closes #10295
Similar to the batching of "shards-started" actions, this PR implements batching of snapshot status updates. This is useful when backing up many indices as the cluster state does not need to be republished as many times.