Conversation
|
Hey @samansmink, |
There was a problem hiding this comment.
Hey @UriSaShavit, thanks for the PR!
I think this PR is a good idea. However I feel there is a bit too much code duplication in its current shape. Right now there are large blocks of code that are fully duplicated.
Also, the use of global variables does not really seems like a very clean approach. We try to avoid using global variables as much as possible. Could you also take a look at cleaning that up?
|
Hey @samansmink, My team and I are planning to use this capability, so if you’d like, I can definitely take a pass at refactoring and reducing the code duplication. If you’re open to it — I’d love some guidance on what you’d like to see or what direction you prefer. |
- Created UCTableCredentialManager singleton class to centralize AWS temp credentials caching logic - Removed ~70 lines of duplicated code between uc_catalog.cpp and uc_table_entry.cpp - Replaced global variables (table_secret_mutexes, mutex_map_mutex, secret_expiration_times) with private members of credential manager - Extracted magic number (5 minutes) into named constant REFRESH_SAFETY_MARGIN_MS - Fixed secret_caching.test to attach catalog with DEFAULT_SCHEMA and correct expected results This addresses PR feedback about code duplication and global variable usage.
- Kept refactored credential manager code (UCTableCredentialManager) - Resolved Makefile conflict by keeping main's format - Updated includes in uc_table_entry.cpp to use uc_utils.hpp - Preserved all refactoring improvements while incorporating upstream changes
|
Hey @samansmink, |
|
Looks great! I've picked up your PR because the rebase + some minor tweaks + test rework is not super straightforward. I reopened a PR with your commits here #63. Thanks @UriSaShavit! |
2 participants
No description provided.