-
Notifications
You must be signed in to change notification settings - Fork 133
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
Move all AtlasDB persisters to the new reusable type #2300
base: develop
Are you sure you want to change the base?
Conversation
Generate changelog in
|
...-error-prone/src/main/java/com/palantir/baseline/errorprone/NonReusableAtlasdbPersister.java
Outdated
Show resolved
Hide resolved
Given that we own the persister API, I would prefer if we updated the API in such a way that it's difficult to get wrong rather than relying on static analysis. |
I'd prefer that too; @jeremyk-91 is there a world where we could flip the expectations/always enforce this? @carterkozak Would you be ok merging this with a plan to get persisters to be safe by default? |
No objections to changing the API in the long term (in fact that's preferable, I remember this bit of defensiveness being somewhat annoying to add) - the main thing is how to get there. If this is likely to have people touch each of their persisters, I'm thinking if we should instead do:
|
651a263
to
d68dac8
Compare
Refactored this to support the new setup (only |
@@ -50,6 +50,7 @@ public class BaselineErrorProneExtension { | |||
"LambdaMethodReference", | |||
"LoggerEnclosingClass", | |||
"LogsafeArgName", | |||
"NonReusableAtlasdbPersister", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this appropriate for this check? We want to move everyone to this new persister given the risks of the old persister
Before this PR
Non-reusable persisters are created for each cell read/write. This causes a lot of object creation, which is particularly problematic when a new object mapper is created,
After this PR
==COMMIT_MSG==
Enforce all AtlasDB persisters to be marked as reusable
==COMMIT_MSG==
Possible downsides?