Implement Time-To-Live (TTL) Support #23

dkz2 · 2023-07-25T19:52:43Z

This PR addresses the need to enhance our Memcached functionality with Time-To-Live (TTL) support. TTL enables setting an expiration time one a key-value pair in the cache, automatically removing the data when it becomes stale. This PR will close #17.

Motivation:
To improve our interactions with Memcached servers in a more dynamic and versatile way. TTL not only improves cache efficiently but also broadens our API capabilities.

Modifications:

Created a TimeToLive enum that represents the Time-To-Live of a MemcachedValue. This can be set to indefinitely or to expiresAt. with the latter taking a ContinuousClock.Instant instant.
Added a timeToLive property to the MemcachedFlags struct allowing for TTL specifications in both our get and set commands.
Implemented a new touch method in our MemcachedConnection that allows for updating the TTL of an existing key without fetching it.
Updated our set method in our MemcachedConnection to include an optional TimeToLive parameter, this allows the user to set a TTL for the key-value pair. If not set this will default to TTL being set to indefinitely.
Added Unit and Integration test.

Result:
With the addition of TTL support, our API now allows users to set an expiration for their key-value pair, improving cache efficiency and breading the capability of our API.

FranzBusch

Looks really good. Some comments here and there

FranzBusch · 2023-07-26T13:47:59Z

Sources/SwiftMemcache/MemcachedConnection.swift

+    /// Update the TTL for a key in the Memcache server.
+    ///
+    /// - Parameters:
+    ///   - key: The key for which to update the TTL.
+    ///   - newTimeToLive: The new Time-To-Live value. If `nil`, the TTL will not be updated.
+    /// - Throws: A `MemcachedConnectionError` if the connection is shutdown or if there's an unexpected nil response.


Suggested change

/// Update the TTL for a key in the Memcache server.

///

/// - Parameters:

/// - key: The key for which to update the TTL.

/// - newTimeToLive: The new Time-To-Live value. If `nil`, the TTL will not be updated.

/// - Throws: A `MemcachedConnectionError` if the connection is shutdown or if there's an unexpected nil response.

/// Fetch the value and set its time-to-live for a key.

///

/// - Parameters:

/// - key: The key to fetch the value and update the time-to-live for.

/// - newTimeToLive: The new time-to-live. If `nil`, the TTL will not be updated.

/// - Throws: A `MemcachedConnectionError` if the connection is shutdown or if there's an unexpected nil response.

FranzBusch · 2023-07-26T13:49:23Z

Sources/SwiftMemcache/MemcachedFlags.swift

+    /// Returns the duration in seconds between the current time and the expiration time.
+    public func durationUntilExpiration(inRelationTo clock: ContinuousClock) -> UInt32 {


I don't think we should provide this. People can write this in their own code if they want to

FranzBusch · 2023-07-26T13:49:57Z

Sources/SwiftMemcache/MemcachedConnection.swift

+    ///   - key: The key for which to update the TTL.
+    ///   - newTimeToLive: The new Time-To-Live value. If `nil`, the TTL will not be updated.
+    /// - Throws: A `MemcachedConnectionError` if the connection is shutdown or if there's an unexpected nil response.
+    public func get<Value: MemcachedValue>(_ key: String, as valueType: Value.Type = Value.self, newTimeToLive: TimeToLive? = nil) async throws {


Why are we taking an option TimeToLive here? Isn't nil = indefinitely?

FranzBusch · 2023-07-26T13:50:38Z

Sources/SwiftMemcache/MemcachedConnection.swift

+        switch self.state {
+        case .initial(_, _, _, let requestContinuation, let clock),
+             .running(_, _, _, let requestContinuation, let clock):
+
+            var flags = MemcachedFlags()
+            if let newTimeToLive {
+                flags = MemcachedFlags()
+                flags.timeToLive = newTimeToLive.durationUntilExpiration(inRelationTo: clock)
+            }
+            flags.shouldReturnValue = true
+
+            let command = MemcachedRequest.GetCommand(key: key, flags: flags)
+            let request = MemcachedRequest.get(command)
+
+            _ = try await withCheckedThrowingContinuation { continuation in
+                switch requestContinuation.yield((request, continuation)) {
+                case .enqueued:
+                    break
+                case .dropped, .terminated:
+                    continuation.resume(throwing: MemcachedConnectionError.connectionShutdown)
+                default:
+                    break
+                }
+            }
+
+        case .finished:
+            throw MemcachedConnectionError.connectionShutdown
+        }


We have been duplicating this code a bunch now. We should write a private method that takes a MemcacheRequest and does the continuation dance for us so that we just have to create the request and get the correct response.

FranzBusch · 2023-07-26T13:52:10Z

Sources/SwiftMemcache/MemcachedConnection.swift

+    /// Fetch the value for a key and its TTL from the Memcache server.
+    ///
+    /// - Parameter key: The key to fetch the value and TTL for.
+    /// - Returns: A tuple containing the fetched value and its TTL, or `nil` if no value was found.
+    /// - Throws: A `MemcachedConnectionError` if the connection is shutdown.


Suggested change

/// Fetch the value for a key and its TTL from the Memcache server.

///

/// - Parameter key: The key to fetch the value and TTL for.

/// - Returns: A tuple containing the fetched value and its TTL, or `nil` if no value was found.

/// - Throws: A `MemcachedConnectionError` if the connection is shutdown.

/// Fetch the value and its time-to-live for a key.

///

/// - Parameter key: The key to fetch the value and time-to-live for.

/// - Returns: A tuple containing the fetched value and its TTL, or `nil` if no value was found.

/// - Throws: A `MemcachedConnectionError` if the connection is shutdown.

FranzBusch · 2023-07-26T13:52:56Z

Sources/SwiftMemcache/MemcachedConnection.swift

+    /// - Parameter key: The key to fetch the value and TTL for.
+    /// - Returns: A tuple containing the fetched value and its TTL, or `nil` if no value was found.
+    /// - Throws: A `MemcachedConnectionError` if the connection is shutdown.
+    public func get<Value: MemcachedValue>(_ key: String, as valueType: Value.Type = Value.self) async throws -> (Value?, TimeToLive?) {


I am not a big fan of the tuple here. What about creating a new struct called ValueAndTimeToLive. At the same time only that thing should be optional and not both. If we get a value we must get a TTL as well.

FranzBusch · 2023-07-26T13:53:03Z

Sources/SwiftMemcache/MemcachedConnection.swift

+                }
+            }
+
+            let value = Value.readFromBuffer(&response.value!)


This force unwrap is not safe

FranzBusch · 2023-07-26T13:54:04Z

Sources/SwiftMemcache/MemcachedConnection.swift

+            let ttl = response.flags?.timeToLive.map {
+                TimeToLive.expiresAt(clock.now.advanced(by: Duration.seconds($0)))
+            } ?? .indefinitely


Do we get a flags.timeToLive back even if there is no TTL set on the key?

Ah, I must have missed this case, as per the protocol
- t: return item TTL remaining in seconds (-1 for unlimited)
if no TTL is set it will default to -1.

FranzBusch · 2023-07-26T13:54:37Z

Sources/SwiftMemcache/MemcachedConnection.swift

+    ///     If set, the value will expire after the specified TTL.
+    ///     If not set, the value will not expire.
+    /// - Throws: A `MemcachedConnectionError` if the connection is shutdown.
+    public func set(_ key: String, value: some MemcachedValue, expiration: TimeToLive? = nil) async throws {


Can we make the expiration non optional and just default to indefinitely.

FranzBusch · 2023-07-26T20:33:12Z

Sources/SwiftMemcache/Extensions/ByteBuffer+SwiftMemcache.swift

+                    if ttlIsUnlimited {
+                        // If TTL is negative, set it as indefinite.
+                        flags.timeToLive = nil
+                        ttlIsUnlimited = false


How do we differentiate the case when TTL is nil because it is not present or when it is nil because it is infinite? I think we should probably store this a bit differently in the MemcacheFlags struct

In our MemcacheFlags we can explore storing timeToLive as our TimeToLive enum (instead of as a UInt32), that way when TTL is present and indefinite we store it as flags.timeToLive = .indefinitely and when TTL is present and has a expiry time we store it as flags.timeToLive = .expiresAt(instant)

That sounds good. We should probably store it as a TimeToLive?

FranzBusch · 2023-07-26T20:35:24Z

Sources/SwiftMemcache/MemcachedFlags.swift

+    public let value: Value
+    /// The TTL of the fetched value.
+    public let ttl: TimeToLive


Can we make those vars instead.

FranzBusch · 2023-07-26T20:35:35Z

Sources/SwiftMemcache/MemcachedFlags.swift

+    /// The value should never expire.
+    case indefinitely
+    /// The value should expire after a specified time.
+    case expiresAt(ContinuousClock.Instant, (ContinuousClock.Instant, ContinuousClock) -> UInt32 = { _, _ in return 0 })


Why do we have a closure here now?

FranzBusch · 2023-07-28T13:56:24Z

Sources/SwiftMemcache/Extensions/ByteBuffer+SwiftMemcache.swift

+        if let timeToLive = flags.timeToLive {
+            switch timeToLive {
+            case .indefinitely:
+                break


I think we have to write something out here. In the case where the item has a TTL and we want to set it to indefinitely now. Please also write a test for this.

FranzBusch · 2023-07-28T14:00:08Z

Sources/SwiftMemcache/MemcachedFlags.swift

+    ///
+    /// If true, the Time-To-Live (TTL) for the item is returned.
+    /// If false, the TTL for the item is not returned.
+    var shouldReturnTTL: Bool?


Suggested change

var shouldReturnTTL: Bool?

var shouldReturnTimeToLive: Bool?

FranzBusch · 2023-07-28T14:00:39Z

Sources/SwiftMemcache/MemcachedFlags.swift

+    /// The TTL of the fetched value.
+    public var ttl: TimeToLive
+
+    /// Initializes a new instance of `ValueAndTimeToLive` with a value and its TTL.
+    public init(value: Value, ttl: TimeToLive) {
+        self.value = value
+        self.ttl = ttl


Suggested change

/// The TTL of the fetched value.

public var ttl: TimeToLive

/// Initializes a new instance of `ValueAndTimeToLive` with a value and its TTL.

public init(value: Value, ttl: TimeToLive) {

self.value = value

self.ttl = ttl

/// The time-to-live of the fetched value.

public var timeToLive: TimeToLive

/// Initializes a new instance of `ValueAndTimeToLive` with a value and its time-to-live.

public init(value: Value, timeToLive: TimeToLive) {

self.value = value

self.ttl = ttl

Sources/SwiftMemcache/MemcachedFlags.swift

FranzBusch · 2023-07-31T08:02:16Z

Sources/SwiftMemcache/MemcachedConnection.swift

+    ///   - key: The key to update the time-to-live for.
+    ///   - newTimeToLive: The new time-to-live.
+    /// - Throws: A `MemcachedConnectionError` if the connection is shutdown or if there's an unexpected nil response.
+    public func touch<Value: MemcachedValue>(_ key: String, as valueType: Value.Type = Value.self, newTimeToLive: TimeToLive) async throws {


Suggested change

public func touch<Value: MemcachedValue>(_ key: String, as valueType: Value.Type = Value.self, newTimeToLive: TimeToLive) async throws {

public func touch(_ key: String, newTimeToLive: TimeToLive) async throws {

Sources/SwiftMemcache/MemcachedConnection.swift

FranzBusch · 2023-07-31T08:03:36Z

Sources/SwiftMemcache/MemcachedConnection.swift

+    ///     If provided, the key-value pair will be removed from the cache after the specified TTL duration has passed.
+    ///     If not provided, the key-value pair will persist indefinitely in the cache.
+    /// - Throws: A `MemcachedConnectionError` if the connection to the Memcached server is shut down.
+    public func set(_ key: String, value: some MemcachedValue, expiration: TimeToLive = .indefinitely) async throws {


We should be consistent here and name this parameter timeToLive then.

FranzBusch · 2023-07-31T08:04:55Z

Sources/SwiftMemcache/MemcachedFlags.swift

+    /// Compares two `MemcachedFlags` instances for equality.
+    ///
+    /// Two `MemcachedFlags` instances are considered equal if they have the same `shouldReturnValue`, and `timeToLive` properties.
+    ///
+    /// - Parameters:
+    ///   - lhs: A `MemcachedFlags` instance.
+    ///   - rhs: Another `MemcachedFlags` instance.
+    /// - Returns: `true` if the two instances are equal, `false` otherwise.
+    static func == (lhs: MemcachedFlags, rhs: MemcachedFlags) -> Bool {
+        guard lhs.shouldReturnValue == rhs.shouldReturnValue else {
+            return false
+        }
+        switch (lhs.timeToLive, rhs.timeToLive) {
+        case (.indefinitely?, .indefinitely?), (nil, nil):
+            return true
+        case (.expiresAt(let lhsInstant)?, .expiresAt(let rhsInstant)?):
+            return lhsInstant == rhsInstant
+        default:
+            return false
+        }
+    }
+
+    /// Provides a hash value for a `MemcachedFlags` instance.
+    ///
+    /// The hash value is composed from the `shouldReturnValue`,  and `timeToLive` properties.
+    ///
+    /// - Parameter hasher: The hasher to use when combining the components of the instance.
+    func hash(into hasher: inout Hasher) {
+        hasher.combine(self.shouldReturnValue)
+        switch self.timeToLive {
+        case .indefinitely:
+            hasher.combine("indefinitely")
+        case .expiresAt(let instant):
+            hasher.combine(instant)
+        case .none:
+            break
+        }
+    }


I don't think have to implement those. The compiler should be able to synthesise them.

FranzBusch · 2023-07-31T08:07:17Z

Tests/SwiftMemcacheTests/IntegrationTest/MemcachedIntegrationTests.swift

+            let initialTTLValue = 5
+            let now = ContinuousClock.Instant.now
+            let expirationTime = now.advanced(by: .seconds(initialTTLValue))
+            let expiration = TimeToLive.expiresAt(expirationTime)
+            try await memcachedConnection.set("bar", value: setValue, expiration: expiration)
+
+            // Update the TTL for the key to indefinite
+            let newExpiration = TimeToLive.indefinitely
+            _ = try await memcachedConnection.touch("bar", as: String.self, newTimeToLive: newExpiration)
+
+            // Wait for more than the initial TTL duration
+            // Sleep for 6 seconds
+            try await Task.sleep(nanoseconds: UInt64(6 * 1_000_000_000))


Let's use a shorter intial TTL here something like 1 second and then let's only sleep for 1.5 seconds after. Otherwise this test takes forever. Also please use Task.sleep(for:) instead of the nanosecond variant.

FranzBusch · 2023-07-31T08:07:52Z

Sources/SwiftMemcache/MemcachedFlags.swift

+/// The 'v' flag for the meta get command dictates whether the item value should be returned in the data block.
+/// The 'T' flag is used for both the meta get and meta set commands to specify the Time-To-Live (TTL) for an item.
+/// The 't' flag for the meta get command indicates whether the Time-To-Live (TTL) for the item should be returned.
+@available(macOS 13.0, *)


We always have to specify all platforms here. Take a look at the Clock availability and just copy it here and everywhere else. Maybe we should just bump our platform versions on Package.swift instead though. That might be easier

Updating our Package.swift!

FranzBusch · 2023-07-31T08:09:25Z

Tests/SwiftMemcacheTests/IntegrationTest/MemcachedIntegrationTests.swift

+            let initialTTLValue = 60 * 60 * 24 * 30 + 5 // 30 days + 5 seconds
+            let now = ContinuousClock.Instant.now
+            let expirationTime = now.advanced(by: .seconds(initialTTLValue))
+            let expiration = TimeToLive.expiresAt(expirationTime)
+            try await memcachedConnection.set("bar", value: setValue, expiration: expiration)
+
+            // Wait for 6 seconds
+            try await Task.sleep(nanoseconds: UInt64(6 * 1_000_000_000)) // Sleep for 6 seconds


Same here. Please use a short TTL and use the other sleep method.

FranzBusch · 2023-07-31T08:09:56Z

Tests/SwiftMemcacheTests/UnitTest/MemcachedRequestEncoderTests.swift

@@ -43,6 +44,29 @@ final class MemcachedRequestEncoderTests: XCTestCase {
        XCTAssertEqual(outBuffer.getString(at: 0, length: outBuffer.readableBytes), expectedEncodedData)
    }

+    func testEncodeSetTTLRequest() {


We should have a test for indefinitely and for a very larger Instant as well (something larger than 30 days)

dkz2 added 6 commits July 23, 2023 22:14

set support for TTL

db5549b

TTL support for get/set

d4b0453

remove debug statements

5e165ea

update doc comments

1e0c5cf

keep our main example the same

4d41ffd

naming consistancy in test

877f485

FranzBusch reviewed Jul 26, 2023

View reviewed changes

PR comment fixes

6c08fcf

FranzBusch reviewed Jul 26, 2023

View reviewed changes

dkz2 added 4 commits July 28, 2023 01:06

refactored flags to hold TimeToLive

2fe6790

indefinitely test

2f08670

naming consistency

cb97a13

TTL conformance for Eq + Hash

d1b3c8c

FranzBusch reviewed Jul 28, 2023

View reviewed changes

dkz2 added 3 commits July 29, 2023 11:24

refactored MemcachedConnection

58dafa5

soundness

c375fe4

remove flgs from set

f9446ea

FranzBusch reviewed Jul 31, 2023

View reviewed changes

dkz2 added 3 commits July 31, 2023 11:40

clean up remaining PR cmnts

8ce9dd3

no longer using these

0ac2c99

closes swift-server#17

d4c0ef5

dkz2 marked this pull request as ready for review July 31, 2023 17:05

FranzBusch approved these changes Aug 1, 2023

View reviewed changes

FranzBusch merged commit 0b17329 into swift-server:main Aug 1, 2023

		/// Returns the duration in seconds between the current time and the expiration time.
		public func durationUntilExpiration(inRelationTo clock: ContinuousClock) -> UInt32 {

	public func touch<Value: MemcachedValue>(_ key: String, as valueType: Value.Type = Value.self, newTimeToLive: TimeToLive) async throws {
	public func touch(_ key: String, newTimeToLive: TimeToLive) async throws {

Implement Time-To-Live (TTL) Support #23

Implement Time-To-Live (TTL) Support #23

Uh oh!

Conversation

dkz2 commented Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FranzBusch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkz2 Jul 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dkz2 commented Jul 25, 2023 •

edited

Loading

dkz2 Jul 27, 2023 •

edited

Loading