[Issue 210] - allow unexpected child shards to be ignored #240
Conversation
now instead of always throwing an assertion if a child shard has an open parent, consider worker configuration before doing so. if configured to ignore such shards, do not create leases for them during shard sync. this is intended to mitigate failing worker init when processing dynamodb streams with many thousands of shards (which can happen for tables with thousands of partitions). this new behavior can be enabled by adding the following to a configuration/properties file: ``` ignoreUnexpectedChildShards = true ```
|
Thanks for submitting this, just a couple of requests: Can you update this with the new changes, and remove the white space changes? I would also like to see some tests to verify that it doesn't break the existing behavior, and tests for the new behavior. |
|
Will do. |
instead of adding the `ignoreUnexpectedChildShards` field to various objects and passing it as an explicit parameter, refrain from adding the field except where needed and obtain the value from the already-passed `KinesisClientLibConfiguration` parameter.
|
@pfifer: Branch has been updated and unrelated whitespace changes removed, and two basic testcases added which exercise the new behavior. Looking at the existing tests, it seems to me they adequately cover the default behavior. |
| @@ -351,6 +359,7 @@ public KinesisClientLibConfiguration(String applicationName, | |||
| long parentShardPollIntervalMillis, | |||
| long shardSyncIntervalMillis, | |||
| boolean cleanupTerminatedShardsBeforeExpiry, | |||
| boolean ignoreUnexpectedChildShards, | |||
There was a problem hiding this comment.
You're changing a public constructor and adding a parameter in the middle of it. This will break anyone who uses the constructor. I would prefer to add a new constructor with the parameter. The other option is not to have the parameter, and use the with/set operations to configure the feature.
At some point we will look into switching to a builder pattern for the configuration.
| for (String id : inconsistentShardIds) { | ||
| ids += " " + id; | ||
| } | ||
| throw new KinesisClientLibIOException(String.valueOf(inconsistentShardIds.size()) + " open child shards (" + ids + ") are inconsistent." |
There was a problem hiding this comment.
This might be cleaner using String.format.
just use a `.properties` file or the appropriate `with` method to set `ignoreUnexpectedChildShards` config param.
|
@pfifer: most recent comments addressed. I removed the config constructor changes to avoid polluting that module (and since my own usage involves only |
| private static void assertAllParentShardsAreClosed(Set<String> inconsistentShardIds) | ||
| throws KinesisClientLibIOException { | ||
| if (!inconsistentShardIds.isEmpty()) { | ||
| String ids = ""; |
There was a problem hiding this comment.
You can use StringUtils#join here instead of manually building the string.
There was a problem hiding this comment.
Updated. I don't normally use Java as has no doubt been apparent. :-)
|
Please confirm that we can use, modify, copy, and redistribute this contribution. Thanks. |
I confirm this to be true. |
* Allow disabling check for the case where a child shard has an open parent shard. There is a race condition where it's possible for the a parent shard to appear open, while having child shards. This check can now be disabled by setting ignoreUnexpectedChildShards in the KinesisClientLibConfiguration to true. * PR awslabs#240 * Issue awslabs#210 * Upgraded the AWS SDK for Java to 1.11.261 * PR awslabs#281
* Allow disabling check for the case where a child shard has an open parent shard. There is a race condition where it's possible for the a parent shard to appear open, while having child shards. This check can now be disabled by setting ignoreUnexpectedChildShards in the KinesisClientLibConfiguration to true. * PR #240 * Issue #210 * Upgraded the AWS SDK for Java to 1.11.261 * PR #281
|
Hey there, and sorry for necro'ing 😆 I got here because we were getting bitten by this exact same issue, and AWS support pointed us at #210 which did solve our issue. But I'm really curious -- @pfifer can you comment on why this setting is not the default? E.g. when would it be relevant for me to care about this exception? Thanks a lot! |
This PR, which currently lacks tests, addresses issue #210.
Now instead of always throwing an assertion if a child shard has an
open parent, consider worker configuration before doing so. If
configured to ignore such shards, do not create leases for them during
shard sync. This is intended to mitigate failing worker init when
processing dynamodb streams with many thousands of shards (which can
happen for tables with thousands of partitions).
This new behavior can be enabled by adding the following to a
configuration/properties file: