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

Fix dynamodb HA lock race #5829

Closed
wants to merge 1 commit into from

Conversation

mahmoudm
Copy link
Contributor

@mahmoudm mahmoudm commented Nov 20, 2018

Fixes #5828

@mahmoudm
Copy link
Contributor Author

I'm still testing this change, opened to start the discussion. I'll ping back when tested fully.

@mahmoudm
Copy link
Contributor Author

mahmoudm commented Dec 1, 2018

I've tested this and deployed it to production and it works as expected.

@kalafut
Copy link
Contributor

kalafut commented Dec 1, 2018

@mahmoudm Thanks for digging in to this. I've reviewed it briefly and it looks reasonable. Can you please add a test to dynambo_test.go (i.e. another special purpose test like testDynamoDBLockTTL) that causes the sequence which should fail without your changes? To run those tests you'll need Docker installed, and to just run the Dynamo ones you can use go test ./physical/dynamodb

@briankassouf briankassouf added this to the 1.0.1 milestone Dec 12, 2018
@jefferai jefferai modified the milestones: 1.0.1, 1.1 Dec 12, 2018
@hashicorp-cla
Copy link

hashicorp-cla commented Jan 15, 2019

CLA assistant check

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes


Mahmoud Abdelsalam seems not to be a GitHub user.
You need a GitHub account to be able to sign the CLA. If you already have a GitHub account, please add the email address used for this commit to your account.

Have you signed the CLA already but the status is still pending? Recheck it.

@briankassouf briankassouf modified the milestones: 1.1, 1.1.1 Mar 1, 2019
@maartenvanderhoef
Copy link
Contributor

Hi @mahmoudm would you be able to find time to finish the PR ?

@kalafut
Copy link
Contributor

kalafut commented Apr 1, 2019

@mahmoudm Thanks for working to help clear the CLA block. It looks like some older commits are under another GitHub ID that's no longer active. Would you mind taking your current code and pushing a new PR, squashed without reference to the old ID? We can then close out this PR and I'll help integrate the new one.

@mahmoudm
Copy link
Contributor Author

mahmoudm commented Apr 1, 2019

should be good now - LMK if this doesn't work

@kalafut
Copy link
Contributor

kalafut commented Apr 1, 2019

@mahmoudm Despite the force-push and the history only showing the one commit, the CLA checker is still referencing the old email. Could you push the work (i.e the same squashed commit, possibly as a new branch in your repo) as a new PR? I think that may be simplest way to appease the CLA checker. Thanks for your patience with this!

@mahmoudm
Copy link
Contributor Author

mahmoudm commented Apr 1, 2019

ok, opened here: #6512

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants