-
Notifications
You must be signed in to change notification settings - Fork 216
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 validator to validate percentage of record change #697
Conversation
|
||
import static java.util.Objects.requireNonNull; | ||
|
||
public class RecordCountChangeValidator implements ValidatorListener { |
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.
Lets add to the javadoc that this only supports types that have a primary key definition
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.
Maybe also throw exception to explicitly notify an incorrect use of the validator?
Why does this only support types with primary key definition?
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 AbstractHollowDataAccessor will throw an exception if the type doesn't have a primary key associated, that works.
Why does this only support types with primary key definition?
Without a primary key we don't know if an add+remove was a modification or not (the way to detect modification is to match added and removed record for primary key)
|
||
import static java.util.Objects.requireNonNull; | ||
|
||
public class RecordCountChangeValidator implements ValidatorListener { |
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.
naming: since we're observing percent change lets call it RecordCountPercentChangeValidator
return new ThresholdBuilder(); | ||
} | ||
|
||
public static class ThresholdBuilder { |
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.
if a use case using the builder pattern only supplies the deletion threshold, i think we treat it as- they dont care about added/updated percent. Those should be pass through.
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 seems already handed since we're initializing the thresholds to -1 👍
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.
maybe this usage can also be documented in javadoc
float updatedPercent = (float) updatedRecordNumber / previousRecordNumber; | ||
|
||
|
||
float addedPercentageThreshold = threshold.addedPercentageThreshold; |
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.
lets make these dynamically configurable for when producers want to let a publish through
return this; | ||
} | ||
|
||
public ThresholdBuilder withAddedPercentageThreshold(float addedPercentageThreshold) { |
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.
since the threshold type is float, some javadoc explaining how the input parameter is interpreted would be helpful here, for e.g. is passing 0.01 treated as 1% or 0.01%? for deleted and updated, we can validate that the input is in the range (0,100]
Also lets add this to hollow user docs https://hollow.how/validation/?h=validato#pre-defined-validators |
65a8f43
to
86a5efa
Compare
86a5efa
to
2721e4d
Compare
No description provided.