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

MSET and MSETNX #206

Merged
merged 4 commits into from
Feb 17, 2018
Merged

MSET and MSETNX #206

merged 4 commits into from
Feb 17, 2018

Conversation

ipapapa
Copy link
Contributor

@ipapapa ipapapa commented Feb 14, 2018

Adding support for multi-key operations like MSET and MSETNX.

@@ -134,7 +134,7 @@ private MultiKeyOperation(final List<String> keys, final OpName o) {
this.op = o;
}

@Override
@Override
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation. I just do not get what is the standard here.

} else {
return connPool.executeWithFailover(new CompressionValueMultiKeyOperation<Long>(Arrays.asList(keysvalues), OpName.MSETNX) {
@Override
public Long execute(final Jedis client, final ConnectionContext state) throws DynoException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are we compressing the values here?

return connPool.executeWithFailover(new CompressionValueMultiKeyOperation<String>(Arrays.asList(keysvalues), OpName.MSET) {
@Override
public String execute(final Jedis client, final ConnectionContext state) throws DynoException {
return client.mset(keysvalues);
Copy link
Contributor

Choose a reason for hiding this comment

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

same, no compression happening here


String[] compressMultiKeyValue(ConnectionContext ctx, String... value);

String decompressValue(ConnectionContext ctx, String value);
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason why this doesnt take a list of strings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In multi-key write operations, we receive String... keyValues which is a combination of keys and values and we return either a String or Long. In multi-key read operations, the API receives a String keys which is only the keys that we want to be read and we return a List of values. The list of values is being transformed through the CollectionUtils.transform to each independent components (this is how you had set it up and it looks good to me). Then we take each value and we decompress. Hence, our compressMultiKeyValue has numerous inputs and has some logic to perform the compression like selecting every second element which is the values. The decompressValue does not have any special logic other than decompressing each value one-by-one

Logger.warn(
"UNABLE to compress [" + value + "] for key [" + getStringKey() + "]; sending value uncompressed");
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

for (int i = 0 ; i < items.size() ; i++) {
if( i%2 == 0 ) {
String value = items.get(i);

Copy link
Contributor

Choose a reason for hiding this comment

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

please maintain that comment about 2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -241,48 +240,56 @@ public String decompressValue(String value, ConnectionContext ctx) {
* <ul>
* <lh>String Operations</lh>
* <li>{@link #mget(String...) MGET}</li>
* <li>{@link #mset(String...) MSET}</li>
* <li>{@link #msetnx(String...) MSETNX}</li>
* </ul>
*
* @param <T>
* the parameterized type
*/
private abstract class CompressionValueMultiKeyOperation<T> extends MultiKeyOperation<T>
Copy link
Contributor

Choose a reason for hiding this comment

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

may be rename this to alternate value Multi key operation etc? just an idea

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not an alternate value. It is a key-value pair.

@shailesh33
Copy link
Contributor

LGTM

@ipapapa ipapapa merged commit 6f4ff28 into master Feb 17, 2018
@ipapapa ipapapa deleted the mset branch February 17, 2018 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants