dschneider-pivotal commented on a change in pull request #7429:
URL: https://github.com/apache/geode/pull/7429#discussion_r821878387
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/AbstractRedisData.java
##########
@@ -85,6 +87,14 @@
private volatile long expirationTimestamp = NO_EXPIRATION;
private static final ThreadLocal<DeltaInfo> deltaInfo = new ThreadLocal<>();
+ public int getVersion() {
Review comment:
why "int" instead of "short"?
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/AbstractRedisData.java
##########
@@ -218,7 +230,14 @@ public void toDelta(DataOutput out) throws IOException {
@Override
public void fromDelta(DataInput in) throws IOException,
InvalidDeltaException {
+ short deltaVersion = DataSerializer.readPrimitiveShort(in);
Review comment:
It seems like it would be better to only ship the deltaVersion if the
deltaType isVersioned. So you would read the enum first, ask it if it is
versioned and if it is read the version of the wire and check it against the
existing. Since many of our delta ops are idempotent why add a short to every
delta?
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/AbstractRedisData.java
##########
@@ -76,6 +76,8 @@
public static final long NO_EXPIRATION = -1L;
+ protected short version = 0;
Review comment:
it seems like you could even make this a "byte" which would help a tiny
bit with the delta performance
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/RedisString.java
##########
@@ -464,13 +464,14 @@ public void handleSetExpiration(SetOptions options) {
////// methods that modify the "value" field ////////////
- protected void valueAppend(byte[] bytes) {
+ protected synchronized void valueAppend(byte[] bytes) {
Review comment:
Since this synchronization is only to coordinate this thread with a
toData thread, and toData only reads the data, I think it would be helpful to
make this sync more focused. You only need to hold the sync around the last two
lines of the method "value = combined; version++".
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/data/AbstractRedisData.java
##########
@@ -76,6 +76,8 @@
public static final long NO_EXPIRATION = -1L;
+ protected short version = 0;
Review comment:
it seems a little cleaner to make this private and have subclasses use
the gettor/settorr
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
