Skip to content

GEODE-9044: Introduce RedisKey as key object for RedisData entries #6146

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.

Sign up for GitHub

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

Jump to bottom
Merged
jdeppe-pivotal merged 9 commits into from
Mar 22, 2021
Merged

GEODE-9044: Introduce RedisKey as key object for RedisData entries #6146

jdeppe-pivotal merged 9 commits into from
Mar 22, 2021

Conversation

@jdeppe-pivotal
Copy link
Contributor

@jdeppe-pivotal jdeppe-pivotal commented Mar 16, 2021

  • RedisKey extends ByteArrayWrapper and adds a routingId integer value.
    This is used by the RedisPartitionResolver to map entries to buckets.

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

  • Has your PR been rebased against the latest commit within the target branch (typically develop)?

  • Is your initial contribution a single, squashed commit?

  • Does gradlew build run cleanly?

  • Have you written or updated unit tests to verify your changes?

  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?

Note:

Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.

@jdeppe-pivotal jdeppe-pivotal added the redis Issues related to the geode-for-redis module label Mar 16, 2021
@jdeppe-pivotal
Copy link
Contributor Author

jdeppe-pivotal commented Mar 16, 2021

I tried to break this up into one commit which has the 'actual' change and the other is switching usage of ByteArrayWrapper to RedisKey where appropriate.

@jdeppe-pivotal jdeppe-pivotal marked this pull request as draft March 16, 2021 16:35
@jdeppe-pivotal jdeppe-pivotal marked this pull request as ready for review March 16, 2021 17:48
nonbinaryprogrammer
nonbinaryprogrammer approved these changes Mar 16, 2021
View reviewed changes
Copy link
Contributor

@nonbinaryprogrammer nonbinaryprogrammer left a comment

Choose a reason for hiding this comment

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

looks good!

@jdeppe-pivotal jdeppe-pivotal changed the title [DRAFT] Refactor all redis key references to use RedisKey GEODE-9044: Introduce RedisKey as key object for RedisData entries Mar 17, 2021
jake-at-work
jake-at-work requested changes Mar 17, 2021
View reviewed changes
@lgtm-com
Copy link

lgtm-com bot commented Mar 18, 2021

This pull request introduces 1 alert when merging 20de8cf into 14e1cfc - view on LGTM.com

new alerts:

  • 1 for Inconsistent equals and hashCode

jake-at-work
jake-at-work requested changes Mar 18, 2021
View reviewed changes
geode-redis/src/main/java/org/apache/geode/redis/internal/data/RedisKey.java Outdated
public void fromData(DataInput in, DeserializationContext context)
throws IOException, ClassNotFoundException {
routingId = DataSerializer.readInteger(in);
crc16 = in.readInt();
Copy link
Contributor

@jake-at-work jake-at-work Mar 18, 2021

Choose a reason for hiding this comment

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

Wondering out loud now if it is even worth serializing the CRC when we could just reconstitute it when deserializing.

@lgtm-com
Copy link

lgtm-com bot commented Mar 18, 2021

This pull request introduces 1 alert when merging 6f0f4d4 into 3b422bb - view on LGTM.com

new alerts:

  • 1 for Inconsistent equals and hashCode

upthewaterspout
upthewaterspout approved these changes Mar 18, 2021
View reviewed changes
Copy link
Contributor

@upthewaterspout upthewaterspout left a comment

Choose a reason for hiding this comment

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

Looks good. With regard to caching the crc16 in the region entry - I agree it probably doesn't need to be serialized. I'm also not sure whether it would be more efficient to cache the hashCode or the CRC16 - I wish we didn't call the PartitionResolver more than once!

@jake-at-work
Copy link
Contributor

jake-at-work commented Mar 18, 2021

Looks good. With regard to caching the crc16 in the region entry - I agree it probably doesn't need to be serialized. I'm also not sure whether it would be more efficient to cache the hashCode or the CRC16 - I wish we didn't call the PartitionResolver more than once!

At some point there is a tipping point were the serialization of 2 bytes is cheaper than the CPU time to recalculate the CRC remotely. Keeping in mind that for Geode that CPU is also trying to process primary updates too. Or would it even need to be calculated remotely if since it is only used for bucket placement, and that is already known when replicating.

Similar question can likely be said for the hash code's 4 bytes. Since to be placed in to correct map slot remotely the hash code must be known so is recalculation on the remote side more costly in CPU than the 4 bytes on the wire?

Bill
Bill requested changes Mar 19, 2021
View reviewed changes
Copy link
Contributor

@Bill Bill left a comment

Choose a reason for hiding this comment

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

Allocation of the new id value for REDIS_KEY looks good.

But I'm requesting changes to rectify the lack of sufficient testing of computation of CRC16 in the RedisKey constructor.

bschuchardt
bschuchardt approved these changes Mar 22, 2021
View reviewed changes
Copy link
Contributor

@bschuchardt bschuchardt left a comment

Choose a reason for hiding this comment

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

I can't comment on the Redis compatibility changes but I see no problem with the change to DataSerializableFixedID (my only code-owner file in this PR)

Bill
Bill approved these changes Mar 22, 2021
View reviewed changes
Copy link
Contributor

@Bill Bill left a comment

Choose a reason for hiding this comment

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

Approved

@jdeppe-pivotal jdeppe-pivotal merged commit 3a9baef into apache:develop Mar 22, 2021
jdeppe-pivotal added a commit to jdeppe-pivotal/geode that referenced this pull request Mar 23, 2021
@jdeppe-pivotal
GEODE-9044: Introduce RedisKey as key object for RedisData entries (a…
065696b 
…pache#6146)

- Refactor all redis key references to use RedisKey
- RedisKey extends ByteArrayWrapper and adds a routingId integer value.
  This is used by the RedisPartitionResolver to map entries to buckets.

(cherry picked from commit 3a9baef)
nabarunnag pushed a commit that referenced this pull request Mar 23, 2021
@jdeppe-pivotal
GEODE-9044: Introduce RedisKey as key object for RedisData entries (#…
f91c577 
…6146) (#6173)

- Refactor all redis key references to use RedisKey
- RedisKey extends ByteArrayWrapper and adds a routingId integer value.
  This is used by the RedisPartitionResolver to map entries to buckets.

(cherry picked from commit 3a9baef)
pivotal-eshu pushed a commit to agingade/geode that referenced this pull request Mar 30, 2021
@jdeppe-pivotal
GEODE-9044: Introduce RedisKey as key object for RedisData entries (a…
cffaae3 
…pache#6146)

- Refactor all redis key references to use RedisKey
- RedisKey extends ByteArrayWrapper and adds a routingId integer value.
  This is used by the RedisPartitionResolver to map entries to buckets.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Bill Bill Bill approved these changes

@upthewaterspout upthewaterspout upthewaterspout approved these changes

@jake-at-work jake-at-work jake-at-work approved these changes

@bschuchardt bschuchardt bschuchardt approved these changes

@nonbinaryprogrammer nonbinaryprogrammer nonbinaryprogrammer approved these changes

@sabbey37 sabbey37 Awaiting requested review from sabbey37

@dschneider-pivotal dschneider-pivotal Awaiting requested review from dschneider-pivotal

Assignees

No one assigned

Labels

redis Issues related to the geode-for-redis module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jdeppe-pivotal @jake-at-work @Bill @upthewaterspout @bschuchardt @nonbinaryprogrammer