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

DynamoDB Leader Monitor: On Next Should Always Provide MasterDescription #692

Merged

Conversation

crioux-stripe
Copy link
Collaborator

This is an extension of @kmg-stripe 's fix in #691 that is ready to merge.

Context

The DynamoDBMasterMonitor had cases where it may propagate a null value when getLatestMaster() is called, despite having some protections against it. I've removed the AtomicReference to the latest and just rely on the BehaviorSubject to return the latest value.

Checklist

  • ./gradlew build compiles code correctly
  • Added new tests where applicable
  • ./gradlew test passes all tests
  • Extended README or added javadocs where applicable

kmg-stripe and others added 2 commits July 14, 2024 20:03
It appears that specifying null to the observer results in a weird terminal condition where
all values after null are not observed.
Copy link

github-actions bot commented Jul 16, 2024

Test Results

535 tests  +1   529 ✅ +1   7m 52s ⏱️ -5s
139 suites ±0     6 💤 ±0 
139 files   ±0     0 ❌ ±0 

Results for commit 49c8edd. ± Comparison against base commit 10c1dd8.

♻️ This comment has been updated with latest results.

@crioux-stripe crioux-stripe merged commit 3431246 into Netflix:master Jul 17, 2024
4 of 5 checks passed
@crioux-stripe
Copy link
Collaborator Author

I don't have tag pushing rights, but if someone could tag this v3.1.1-rc.1 that would be awesome!

@Andyz26
Copy link
Collaborator

Andyz26 commented Jul 17, 2024

I don't have tag pushing rights, but if someone could tag this v3.1.1-rc.1 that would be awesome!

@crioux-stripe I think @kmg-stripe was able to push the RC tag, and you have the same role in settings. Can you try pushing the tag and let me know if you get any errors?

@crioux-stripe
Copy link
Collaborator Author

@Andyz26 I did think it was kind of odd but I'm getting:

crioux@st-crioux1 mantis % git push origin tag v3.1.1-rc.1
ERROR: Permission to netflix/mantis.git denied to crioux-stripe.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
crioux@st-crioux1 mantis % cat .git/config
[core]
	repositoryformatversion = 0
	filemode = true
	bare = false
	logallrefupdates = true
	ignorecase = true
	precomposeunicode = true
[remote "origin"]
	url = [email protected]:netflix/mantis.git
	fetch = +refs/heads/*:refs/remotes/origin/*
[branch "master"]
	remote = origin
	merge = refs/heads/master

@Andyz26
Copy link
Collaborator

Andyz26 commented Jul 17, 2024

@crioux-stripe, hmm, this is weird: "fatal: Could not read from remote repository." Maybe try reattaching the public SSH keys?
I just checked again, and there is no difference between your account setting and Kevin's. @sarahwada-stripe also got the same permission role. Does it work for you?

@crioux-stripe
Copy link
Collaborator Author

@Andyz26

That did the trick. The default Stripe one appeared to be an older RSA key. I added a new ed25519 key and everything appears to work:

crioux@st-crioux1 mantis % git push
Everything up-to-date

(This used to produce an error.)

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.

3 participants