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 TTL support for compressedBucketWrite fn #2930
Add TTL support for compressedBucketWrite fn #2930
Changes from 9 commits
da1267c
7dcbefb
7da6545
e04205a
187f593
d0258c5
6cfe10a
019f66f
6033d9f
aa0bc5f
3d60083
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.
Normally helper functions with this naming convention should be private.
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 checked other fns (ex:
doCreate
,doSet
and also the other invocation ofdoUpdate
) and they are having public as the scope.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.
Normally if the fuc is not used outside class, we would set as private.
Also more importantly, it is a bit odd to allow user to set
ttl
but not actually update ttl inupdate
. If user provide a newttl
value and the node already exit, this function will returnRetCode.OK
without actually change anything.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.
as I said earlier there are several instances of
doCreate
,doSet
,doUpdate
which are declared public. I followed the similar convention. I am not familiar with the helix code base, this is my first PR. If you can guide me what exact change to make here, happy to correct it.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.
+1 on @xyuanlu.
I can understand TTL for create. But why we need TTL for update? The right operation is to let the user delete the node and recreate with a TTL.
I am not confortable with this feature. Before we have strong justification. I dont think we should let this in.
@jacoblukose It would be good to figure out why you would like to support 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.
I feel like this might not be the best approach, but I am OK as long as @junkaixue feels OK. :)
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 am not buying this. As I said, when a node is created, we should not update its property and change it from a persistent node to TTL node.
If you need to do this, then it should be the application logic to make that smoothly migrate from Non TTL to TTL. Also, since this is the API support Helix. I dont see any use cases in HELIX to leverage this.
Accessor is Helix internal thing. If you would like to leverage for company specific feature and not necessary help Helix, please fork the repo and implement it internally.
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.
@xyuanlu thanks for reponding and on the ack. It will be helpful if you can also callout alternative approach here to improve the craft if you thinks thats seen comprised. Happy to correct it.
@junkaixue thanks for responding, would you be kind enough to highlight the code part where the said concern:
I am not buying this. As I said, when a node is created, we should not update its property and change it from a persistent node to TTL node.
is.doUpdate
fn is not going to update ttl for an existing node, only when underlyingcreate
is called, it leveragettl
passed if any. And also, if you see, public facingdoUpdate
doesnt havettl
option exposed. It would be great if you can suggest an alternative on how this need to handled as well.Also I think there is some misunderstanding, the usecase is not to change znodes created with persistent mode to migrate to use ttl mode. Idea is to just expand the scope of
ZkBucketDataAccessor
to leverage zk ttl feature when the same is being initialized.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.
To be honest I think more time is needed when design the ZK update API to keep consistency. I dont have an solid idea in mind how. But I think the fundamental rule is keep the behavior simple, consistent and not ambiguous.
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 @xyuanlu the only reason why
doUpdate
need a ttl value to be passed in is because the current code is using adoCreate
fn inside thedoUpdate
. If there is nodoCreate
call made withindoUpdate
then there is no need to pass in the ttl as well, reason being,ttl
can only be enforced during creation of znode.the related q is, is it a correct behaviour on the existing
doUpdate
fn code to call adoCreate
internally? If not, this should be removed, and application need to handle it by issuing an explicitdoCreate
call. if its correct, then comes to the challenge on how to pass in ttl value to the internaldoCreate
fn insidedoUpdate
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.
note(myself): based on how pr discussion concludes, we might need to use explicit create for first time and then update calls from second time onwards.
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.
note(myself): I think this update() calls also need to be changed to either
create
if first time and thenupdate
for later updates. Or move todoUpdate
fn withttl
support, based on how the PR discussion concludes.