Skip to content
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

[Compatibility] Adding EXPIRETIME and PEXPIRETIME command #664

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Vijay-Nirmal
Copy link
Contributor

Adding EXPIRETIME and EXPIRETIME command to garnet

  • Add EXPIRETIME and EXPIRETIME command
  • Add Test cases
  • Add documentation

Open Question:

  1. For Object type, Should I create a single GarnetObjectType called Expiration instead of EXPIRETIME and PEXPIRETIME? The advantage will be that it will be a single generic implementation that can be reused for others in the future if required. The disadvantage is that there will be a slight performance penalty but no additional memory needed since values of Expiration, EXPIRETIME and PEXPIRETIME are long type, so we can do in-place override the value after recomputing the required value.

@TalZaccai TalZaccai self-requested a review September 18, 2024 18:26
@TalZaccai TalZaccai changed the title [Compatibility] Adding EXPIRETIME and EXPIRETIME command [Compatibility] Adding EXPIRETIME and PEXPIRETIME command Sep 19, 2024
@TalZaccai
Copy link
Contributor

Adding EXPIRETIME and EXPIRETIME command to garnet

  • Add EXPIRETIME and EXPIRETIME command
  • Add Test cases
  • Add documentation

Open Question:

  1. For Object type, Should I create a single GarnetObjectType called Expiration instead of EXPIRETIME and PEXPIRETIME? The advantage will be that it will be a single generic implementation that can be reused for others in the future if required. The disadvantage is that there will be a slight performance penalty but no additional memory needed since values of Expiration, EXPIRETIME and PEXPIRETIME are long type, so we can do in-place override the value after recomputing the required value.

There is already a GarnetObjectType.Expire, why not use that one?

Copy link
Contributor

@TalZaccai TalZaccai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! Added a couple of comments.

/// <summary>
/// Special type indicating EXPIRETIME command
/// </summary>
Expiretime = 0xf9,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think that there's a need for these new types

public unsafe GarnetStatus EXPIRETIME<TContext, TObjectContext>(ref SpanByte key, StoreType storeType, ref SpanByteAndMemory output, ref TContext context, ref TObjectContext objectContext, bool milliseconds = false)
where TContext : ITsavoriteContext<SpanByte, SpanByte, SpanByte, SpanByteAndMemory, long, MainSessionFunctions, MainStoreFunctions, MainStoreAllocator>
where TObjectContext : ITsavoriteContext<byte[], IGarnetObject, ObjectInput, GarnetObjectStoreOutput, long, ObjectSessionFunctions, ObjectStoreFunctions, ObjectStoreAllocator>
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not simply call TTL and convert it to unix-time?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants