Skip to content

Implement an Arithmetic Request supporting increment and decrement operations #29

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

Merged

Conversation

dkz2
Copy link
Contributor

@dkz2 dkz2 commented Aug 8, 2023

This PR extends our Memcached functionality by introducing increment and decrement operations. This addition allows users to perform essential arithmetic task, crucial for diverse caching scenarios and furnishes a more varsity interface.

Motivation:
To enhance interactions with Memcached servers, ensuring both versatilely and dynamics. The introduction of increment and decrement operations address a vital need in caching scenarios, facilitating direct arithmetic adjustments and expanding our API's capabilities.

Modifications:

  • Added a new ArithmeticCommand to our MemcachedRequest struct enabling arithmetic operations in upcoming requests.
  • Updated our MemcachedRequestEncoder to handle arithmetic operation request.
  • MemcachedConnection now boasts increment and decrement methods, providing developers with a straightforward way to interact with the Memcached server for arithmetic processes.
  • Created a ArithmeticMode enum to clearly differential between increment and decrement operations.
  • Added arithmeticDelta property to MemcachedFlags which designates the magnitude by which a value should be incremented or decremented.
  • Added Unit and Integration test.

Results:
With the addition of the increment and decrement operation support, our API now allows users to directly manipulate numerical values stored within Memcached servers. This fosters more dynamic caching strategies and enhances overall data management efficiency.

@dkz2 dkz2 marked this pull request as ready for review August 8, 2023 20:12
@@ -101,6 +101,22 @@ extension ByteBuffer {
self.writeInteger(UInt8.R)
}
}

if let arithmeticMode = flags.arithmeticMode {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you can't specify a storage mode and arithmetic mode. Can we add a precondition somewhere to check for this

/// - key: The key for the value to increment.
/// - amount: The `UInt64` amount to increment the value by.
/// - Throws: A `MemcachedConnectionError` if the connection to the Memcached server is shut down.
public func increment(_ key: String, amount: UInt64) async throws {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use Int here and precondition that it is larger than 0

/// - key: The key for the value to decrement.
/// - amount: The `UInt64` amount to decrement the value by.
/// - Throws: A `MemcachedConnectionError` if the connection to the Memcached server is shut down.
public func decrement(_ key: String, amount: UInt64) async throws {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here and adjust the doc comment as well please

/// Flag 'D' for the 'ma' (meta arithmetic) command.
///
/// Represents the delta to apply to the 'ma' command. The default value is 1.
var arithmeticDelta: UInt64?
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this an associated value of the arithmetic mode enum please

@@ -33,14 +33,15 @@ extension MemcachedValue where Self: FixedWidthInteger {
///
/// - Parameter buffer: The ByteBuffer to which the integer should be written.
public func writeToBuffer(_ buffer: inout ByteBuffer) {
buffer.writeInteger(self)
buffer.writeString(String(self))
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems wrong. If you want to store an integer we shouldn't convert it to a string. Is this the problem you encountered where you set a key and then want to increment it but the type in memcache was wrong?

Copy link
Contributor Author

@dkz2 dkz2 Aug 8, 2023

Choose a reason for hiding this comment

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

yes, for example when I called set with a value of say 100 the request that was being written looked like ms decrement 8 ... when it should look something like ms decrement 3 ... when I went to increment the value I would get this error CLIENT_ERROR cannot increment or decrement non-numeric value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we could also make use of writeIntegerAsASCII here.

Copy link
Contributor

Choose a reason for hiding this comment

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

So the root problem here is that memcache is operating on the values interpreted as ASCII and not the raw byte values right? So decrement expects that the value for a key is an ASCII string representing an integer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that is correct, since the protocol is text based its wanting to receive the values as ASCII strings. we can fall back on making use of writeIntegerAsASCII and then implement one of the possible solutions we outlined here #9 to improve the performance.

@dkz2 dkz2 changed the title Implement arithmetic incr decr support Implement an Arithmetic Request supporting increment and decrement operations Aug 9, 2023
@dkz2 dkz2 changed the title Implement an Arithmetic Request supporting increment and decrement operations Implement an Arithmetic Request supporting increment and decrement operations Aug 9, 2023
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