Check snapshot status before delete index #36
Conversation
| @@ -48,6 +48,7 @@ | |||
| // @see https://github.com/elastic/elasticsearch/blob/master/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java#L108 | |||
| private final long maxIndexNameBytes = 255; | |||
| private final List<Character> inalidIndexCharaters = Arrays.asList('\\', '/', '*', '?', '"', '<', '>', '|', '#', ' ', ','); | |||
| private final long maxSnapshotWaitingTime = 1800; | |||
There was a problem hiding this comment.
What is the unit of this variable? Where does 1800 come from?
There was a problem hiding this comment.
I changed a little a80c72a
I don't have enough confident about this value.
It should depends on Es server's specs and repository types(S3/Local FileSystem, HDFS...).
I don't think HDFS isn't a majority, though...
Then to make configurable is one idea.
As one of the guideline, Elastic Cloud takes snapshot at every 30 minutes to S3 repository.
There was a problem hiding this comment.
30 mins sounds reasonable for me. Can I ask you that it's good idea to parameterize this field as config option?
| sendRequest(indexName, HttpMethod.DELETE, task, retryHelper); | ||
| log.info("Deleted Index [{}]", indexName); | ||
| } | ||
| } | ||
|
|
||
| private void getSnapshotStatusUntilDone(PluginTask task, Jetty92RetryHelper retryHelper) |
There was a problem hiding this comment.
minor: "wait" is better name of this method. Because someone've taken snapshots. Instead, the plugin waits the snapshots completed. How about this?
There was a problem hiding this comment.
I changed to "waitSnapshot" 9290e7f
How about this?
| } | ||
| execCount++; | ||
| totalWaitingTime += 1000 * execCount; | ||
| if (totalWaitingTime > maxSnapshotWaitingTime) { |
There was a problem hiding this comment.
It seems that this retry logic doesn't work. Because totalWaitingTime becomes 2000 after a loop body is executed first time. And please explain why you think that you don't take RetryExecutor provided by Embulk.
There was a problem hiding this comment.
Sorry I changed logic a80c72a
And please explain why you think that you don't take
RetryExecutorprovided by Embulk.
I thought RetryExecutor is too heavy for this purpose.
I just need exponential backoff mechanism but doesn't need exception handling or some others.
isSnapshotProgressing calls sendRequest and it uses RetryExecutor
There was a problem hiding this comment.
I see. Can I ask you to put several comment about it within the source code? And please file new ticket for Embulk's RetryExecutor improvement:)
There was a problem hiding this comment.
Added comment in the code 61601d7 and will create Issue later.
|
@sakama thank you for fixing. It looks good to me. but, please check my comments and fix them if needed. |
|
@sakama thank you! |
Fix to solve #35