Skip to content
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

Get latest counter before attempting leaseSteal to ensure leaseSteal succeeds #765

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Renjuju
Copy link
Contributor

@Renjuju Renjuju commented Dec 17, 2020

Changes

  • Get current lease counter to ensure takeLeases will succeed

Tests

image

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@Renjuju Renjuju changed the title Get latest counter before attempting a take to ensure take succeeds Get latest counter before attempting leaseSteal to ensure leaseSteal succeeds Dec 17, 2020
@@ -48,6 +47,7 @@
import software.amazon.kinesis.metrics.MetricsLevel;
import software.amazon.kinesis.metrics.MetricsScope;
import software.amazon.kinesis.metrics.MetricsUtil;
import static software.amazon.kinesis.common.CommonCalculations.*;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid wildcard in packages

private final MetricsFactory metricsFactory;

private final double RENEWAL_SLACK_PERCENTAGE = 0.5;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static final?

try {
return leaseRefresher.getLease(lease.leaseKey());
} catch (DependencyException | InvalidStateException | ProvisionedThroughputException e) {
log.debug("Unable to retrieve the current lease, defaulting to existing lease", e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make this warn

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also give more context on when this failed. Like while we tried to update the stale leases

Copy link
Contributor

@ashwing ashwing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left few minor comments. I verified the comments in ashwing#94, addressed

@ashwing
Copy link
Contributor

ashwing commented Jan 3, 2022

Thanks I need to go over the PR one more time. Can you do a functional test for this change and update the CR?

Yu Zeng 10:33 AM
sure, just to confirm tha: does the functional test mean Testing KCL in the local?

Ashwin Giridharan 10:34 AM
yeah basically you need to run two instances locally, with different worker identifier
10:34
so that one can steal leases from another (edited)

10:35
when you artificially introduce delay while fetching leases, the worker should attempt to refresh the lease (as per this PR
) and succeed stealing the lease
10:36
Also we should verify new leases are created every lease renewal i.e we should use isMarkedForSteal for only one cycle. (edited)

Yu Zeng 10:37 AM
got it, thanks!

@stair-aws stair-aws added the v2.x Issues related to the 2.x version label Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v2.x Issues related to the 2.x version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants