Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add data compaction policy validator #1238
Add data compaction policy validator #1238
Changes from 5 commits
349fe9f
0b6929d
4922476
d629ba9
29d14ad
22a4b12
4e9dcfb
45a3624
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This may be a follow up: Do we need to validate data compaction configs (if they present). For example,
target_file_size_bytes
needs to be a value larger than 0.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.
We cannot know the semantic within the config map. User can give a string to
target_file_size_bytes
, it should still be acceptable, astarget_file_size_bytes
doesn't mean anything in the schema.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.
Thanks for the explanation! That make sense, we only validate fields like
max_orphan_file_age_in_days
, which is in orphan-file-removal's schemaThere 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.
Now that we have "generic tables", maybe a keyword other that "generic" would be a good idea
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.
How about a name like
PolicyValidators
? This is one of Java code practice to gather static functionalities for a group of objects.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.
Yeah, that sounds good to me. Or it's almost like a factory, right?
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.
I think we can utilize the
getPolicyType()
here. The method will return a type or throw an error for corrupted PolicyEntity. So we can remove the Preconditions check belowThere 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.
That's my first version actually, but the issue is that Java
switch
clause doesn't support a regular class. Here is the error message: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.
Got it, thanks for the explanation!
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.
Wondering if we should use the name of policy type in the
switch
clause, as customized type won't presented as an enum.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.
I think for custom type we should rely on the
default
case to load their corresponding validator impl?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.
we still need to distinguish the different customized types, right?
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.
Just check my understanding: in the future when we add support for custom type, we will need to load the custom type's validator here?
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.
yeah, something like this:
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.
Just brainstorming about the name, may be "isAttachable"?
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.
The method’s primary role is to decide whether a policy is capable of being attached to a target, "canAttach" can be appropriate. I think
isAttachable
is more suitable if the method only takes a policy entity. WDYT?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.
same here
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.
It looks like this is primarily a parser, not a validator if it's turning a string into a policy type.
Also, is there not a shared supertype for policies? Even a marker interface might be a good idea.
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.
Yeah, that's another option I thought of. I'm fine with either way. Are you suggesting to use a new policy interface to avoid type parameters()?
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.
I was imagining something like:
This way I can just write my implementations like this:
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.
Separated the parsing and validating in the new commit. The parsing is with the policy content class now.
Added a marker interface
PolicyContent
.fromString()
is a static method, so I didn't use this pattern, https://en.wikipedia.org/wiki/Curiously_recurring_template_pattern. We could have a pattern likePolicy<T extends Policy<T>>
once we need a inheritable method that could return a subtype.