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

feat: Make bigtable calls retryable #591

Merged
merged 24 commits into from
Feb 27, 2024
Merged

feat: Make bigtable calls retryable #591

merged 24 commits into from
Feb 27, 2024

Conversation

jrconlin
Copy link
Member

@jrconlin jrconlin commented Jan 31, 2024

Closes SYNC-4085

@jrconlin jrconlin requested a review from pjenvey January 31, 2024 20:04
@jrconlin jrconlin requested a review from pjenvey February 27, 2024 01:18
autopush-common/src/db/bigtable/bigtable_client/mod.rs Outdated Show resolved Hide resolved
autopush-common/src/db/bigtable/bigtable_client/mod.rs Outdated Show resolved Hide resolved
.conn
.mutate_row_async_opt(&req, call_opts(self.metadata.clone()))
.map_err(error::BigTableError::Write)?
retry_policy(self.settings.retry_count)
Copy link
Member

Choose a reason for hiding this comment

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

Should make this a method

Suggested change
retry_policy(self.settings.retry_count)
self.retry_policy()

Copy link
Member Author

Choose a reason for hiding this comment

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

(Remembers why it's not):
I use the same retry_policy for the heath_check which doesn't take a BigClientTableImpl.
I may need to restructure things a bit to do that.

Copy link
Member

Choose a reason for hiding this comment

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

I noticed that, I'm fine with a one off for it. We have an open issue to rework the health check so that whole method will probably change quite a bit sooner rather than later anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, gonna push back on this one.
The function takes one parameter, and we're using it in different paths, so making this dependent on BigTableClientImpl would over complicate things right now.

We might want to revisit the structure of all of this later, but that should be a different ticket.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, there's a lot of "Get It Working" in this. There's lots of room for fit and finish later.

autopush-common/src/db/bigtable/mod.rs Outdated Show resolved Hide resolved
autopush-common/src/db/bigtable/bigtable_client/mod.rs Outdated Show resolved Hide resolved
let r = self
.conn
.read_rows_opt(&req, call_opts(self.metadata.clone()))
let r = retry_policy(10)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let r = retry_policy(10)
let r = retry_policy(5)

pjenvey
pjenvey previously approved these changes Feb 27, 2024
@pjenvey pjenvey merged commit 12dd7b4 into master Feb 27, 2024
1 check passed
@pjenvey pjenvey deleted the feat/SYNC-4085_retry branch February 27, 2024 23:11
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.

2 participants