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

bigtable: remove extra clones #34239

Merged
merged 1 commit into from
Dec 20, 2023

Conversation

fanatid
Copy link
Contributor

@fanatid fanatid commented Nov 28, 2023

extracted from #34213

Right now we clone AccessToken on every request to BigTable, also we clone it when making a token refresh. Inside AccessToken we clone Credentials struct which contains few strings and also clone 2 Arc. We can wrap everything into one Arc and significantly reduce the number of clones.

@mergify mergify bot added community Community contribution need:merge-assist labels Nov 28, 2023
@mergify mergify bot requested a review from a team November 28, 2023 00:26
@github-actions github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Dec 12, 2023
@github-actions github-actions bot closed this Dec 20, 2023
@fanatid
Copy link
Contributor Author

fanatid commented Dec 20, 2023

@CriesofCarrots what can I do to make it merged? We do not need to clone Credentials with a lot of Strings inside :/

@CriesofCarrots
Copy link
Contributor

Ohai. I assumed you changed your mind on this, since you didn't include it as a separate commit as I suggested.
Can you please actually fill out the PR description to justify the change? Thanks

@CriesofCarrots
Copy link
Contributor

Also, looks like you'll need to rebase to pick up the audit fix

@fanatid fanatid force-pushed the bt-remove-extra-clones branch from ad8cc3d to 1f17f4e Compare December 20, 2023 20:57
@fanatid
Copy link
Contributor Author

fanatid commented Dec 20, 2023

Updated!

@CriesofCarrots CriesofCarrots added automerge Merge this Pull Request automatically once CI passes and removed stale [bot only] Added to stale content; results in auto-close after a week. labels Dec 20, 2023
Copy link

codecov bot commented Dec 20, 2023

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (d436352) 81.8% compared to head (1f17f4e) 81.8%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master   #34239     +/-   ##
=========================================
- Coverage    81.8%    81.8%   -0.1%     
=========================================
  Files         822      822             
  Lines      221575   221573      -2     
=========================================
- Hits       181440   181342     -98     
- Misses      40135    40231     +96     

@mergify mergify bot merged commit 1db76cf into solana-labs:master Dec 20, 2023
36 checks passed
@fanatid fanatid deleted the bt-remove-extra-clones branch December 20, 2023 23:21
@fanatid
Copy link
Contributor Author

fanatid commented Dec 20, 2023

Thank you! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge this Pull Request automatically once CI passes community Community contribution need:merge-assist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants