You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "dcapwell (via GitHub)" <gi...@apache.org> on 2025/03/05 23:54:43 UTC
[PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
dcapwell opened a new pull request, #3954:
URL: https://github.com/apache/cassandra/pull/3954
(no comment)
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1985837304
##########
src/java/org/apache/cassandra/service/accord/serializers/IVersionedSerializer.java:
##########
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.service.accord.serializers;
+
+import java.io.IOException;
+
+import org.apache.cassandra.io.Serializer;
+import org.apache.cassandra.io.VersionedSerializer;
+import org.apache.cassandra.io.util.DataInputPlus;
+import org.apache.cassandra.io.util.DataOutputPlus;
+
+public interface IVersionedSerializer<T> extends VersionedSerializer<T, Version>
Review Comment:
wondering if we should rename this to something like `AccordVersionedSerializer`? It has this name as it was easier to bulk migrate with `awk` this way
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1990401560
##########
src/java/org/apache/cassandra/service/accord/api/TokenKey.java:
##########
@@ -149,16 +146,6 @@ public String toString()
return prefix() + ":" + printableSuffix();
}
- public boolean isMin()
Review Comment:
just moved to the same place as the other `is*` methods
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1985882439
##########
src/java/org/apache/cassandra/utils/Int32Serializer.java:
##########
@@ -22,25 +22,44 @@
import org.apache.cassandra.db.TypeSizes;
import org.apache.cassandra.io.IVersionedSerializer;
+import org.apache.cassandra.io.Serializer;
import org.apache.cassandra.io.util.DataInputPlus;
import org.apache.cassandra.io.util.DataOutputPlus;
-public class Int32Serializer implements IVersionedSerializer<Integer>
+public class Int32Serializer implements Serializer<Integer>, IVersionedSerializer<Integer>
Review Comment:
it was either have both or change `ParamType`
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1982393798
##########
src/java/org/apache/cassandra/net/Verb.java:
##########
@@ -630,6 +634,16 @@ private static int idForCustomVerb(int id)
{
return CUSTOM_VERB_START - id;
}
+
+ private static <A, B> IVersionedAsymmetricSerializer<A, B> accordEmbedded(AsymmetricVersionedSerializer<A, B, Version> delegate)
Review Comment:
something now properly handled in this patch is figuring out what version to use. In some code paths we have relied on node version to infer what capabilities are present. There is a ML thread about adding capabilities into C*, which would let us denote what versions are supported by which notes... for now this logic is mostly a place holder
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1992461781
##########
test/unit/org/apache/cassandra/service/accord/serializers/TopologySerializersTest.java:
##########
@@ -18,29 +18,43 @@
package org.apache.cassandra.service.accord.serializers;
+import java.io.IOException;
+
import org.junit.Test;
import accord.local.Node;
+import accord.utils.AccordGens;
import org.apache.cassandra.config.DatabaseDescriptor;
-import org.apache.cassandra.utils.SerializerTestUtils;
+import org.apache.cassandra.io.Serializers;
+import org.apache.cassandra.io.util.DataOutputBuffer;
+import org.apache.cassandra.utils.AccordGenerators;
+
+import static accord.utils.Property.qt;
public class TopologySerializersTest
{
static
{
- DatabaseDescriptor.toolInitialization();
+ DatabaseDescriptor.clientInitialization();
}
@Test
- public void nodeId()
+ public void nodeId() throws IOException
{
- SerializerTestUtils.assertSerializerIOEquality(new Node.Id(1234567890), TopologySerializers.nodeId);
+ qt().forAll(AccordGens.nodes()).check(n -> Serializers.testSerde(TopologySerializers.nodeId, n));
}
@Test
- public void requestScopeTest()
+ public void topology()
{
-
+ @SuppressWarnings({ "resource", "IOResourceOpenedButNotSafelyClosed" }) DataOutputBuffer output = new DataOutputBuffer();
+ qt().forAll(AccordGenerators.partitioner().flatMap(p -> AccordGenerators.topologyGen(p))).check(expected -> {
+ AccordGenerators.maybeUpdatePartitioner(expected.ranges());
+ Serializers.testSerde(output, TopologySerializers.topology, expected);
+
+ for (Node.Id node : expected.nodes())
+ Serializers.testSerde(output, TopologySerializers.topology, expected.forNode(node));
Review Comment:
serializer didn't properly handle this case
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1982388348
##########
src/java/org/apache/cassandra/io/IVersionedSerializer.java:
##########
@@ -17,6 +17,34 @@
*/
package org.apache.cassandra.io;
+import java.io.IOException;
+
+import org.apache.cassandra.io.util.DataInputPlus;
+import org.apache.cassandra.io.util.DataOutputPlus;
+
public interface IVersionedSerializer<T> extends IVersionedAsymmetricSerializer<T, T>
{
+ static <T> IVersionedSerializer<T> fromSerializer(Serializer<T> delegate)
Review Comment:
TODO: this API isn't consistent, should look closer to make sure the APIs match for each interface
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "ifesdjeen (via GitHub)" <gi...@apache.org>.
ifesdjeen commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1994018903
##########
src/java/org/apache/cassandra/service/accord/FetchTopologies.java:
##########
@@ -91,42 +91,42 @@ public FetchTopologies(long minEpoch, long maxEpoch)
}
// TODO (required): messaging version after version patch
- public static final IVersionedSerializer<TopologyRange> responseSerializer = new IVersionedSerializer<>()
+ public static final UnversionedSerializer<TopologyRange> responseSerializer = new UnversionedSerializer<>()
Review Comment:
TODO above can be removed since we decided to keep unversioned serializer here.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1992437121
##########
src/java/org/apache/cassandra/service/accord/journal/AccordTopologyUpdate.java:
##########
@@ -55,178 +56,181 @@ static AccordTopologyUpdate newTopology(Journal.TopologyUpdate update)
{
return new NewTopology(update);
}
- class RangesForEpochSerializer implements IVersionedSerializer<CommandStores.RangesForEpoch>
+ class RangesForEpochSerializer implements org.apache.cassandra.io.Serializer<CommandStores.RangesForEpoch>
{
public static final RangesForEpochSerializer instance = new RangesForEpochSerializer();
@Override
- public void serialize(CommandStores.RangesForEpoch from, DataOutputPlus out, int version) throws IOException
+ public void serialize(CommandStores.RangesForEpoch from, DataOutputPlus out) throws IOException
{
out.writeUnsignedVInt32(from.size());
- from.forEach((epoch, ranges) -> {
- try
- {
- out.writeLong(epoch);
- KeySerializers.ranges.serialize(ranges, out, version);
- }
- catch (Throwable t)
- {
- throw new IllegalStateException("Serialization error", t);
- }
- });
+ for (int i = 0; i < from.size(); i++)
+ {
+ out.writeLong(from.epochAtIndex(i));
+ KeySerializers.ranges.serialize(from.rangesAtIndex(i), out);
+ }
}
@Override
- public CommandStores.RangesForEpoch deserialize(DataInputPlus in, int version) throws IOException
+ public CommandStores.RangesForEpoch deserialize(DataInputPlus in) throws IOException
{
int size = in.readUnsignedVInt32();
Ranges[] ranges = new Ranges[size];
long[] epochs = new long[size];
for (int i = 0; i < ranges.length; i++)
{
epochs[i] = in.readLong();
- ranges[i] = KeySerializers.ranges.deserialize(in, version);
+ ranges[i] = KeySerializers.ranges.deserialize(in);
}
Invariants.require(ranges.length == epochs.length);
return new CommandStores.RangesForEpoch(epochs, ranges);
}
@Override
- public long serializedSize(CommandStores.RangesForEpoch t, int version)
+ public long serializedSize(CommandStores.RangesForEpoch from)
{
- return TypeSizes.sizeofUnsignedVInt(t.size());
+ long size = TypeSizes.sizeofUnsignedVInt(from.size());
+ for (int i = 0; i < from.size(); i++)
+ {
+ size += TypeSizes.sizeof(from.epochAtIndex(i));
+ size += KeySerializers.ranges.serializedSize(from.rangesAtIndex(i));
+ }
+ return size;
}
}
- class TopologyUpdateSerializer implements IVersionedSerializer<Journal.TopologyUpdate>
+ class TopologyUpdateSerializer implements org.apache.cassandra.io.Serializer<Journal.TopologyUpdate>
{
public static final TopologyUpdateSerializer instance = new TopologyUpdateSerializer();
@Override
- public void serialize(Journal.TopologyUpdate from, DataOutputPlus out, int version) throws IOException
+ public void serialize(Journal.TopologyUpdate from, DataOutputPlus out) throws IOException
{
out.writeUnsignedVInt32(from.commandStores.size());
for (Map.Entry<Integer, CommandStores.RangesForEpoch> e : from.commandStores.entrySet())
{
out.writeUnsignedVInt32(e.getKey());
- RangesForEpochSerializer.instance.serialize(e.getValue(), out, version);
+ RangesForEpochSerializer.instance.serialize(e.getValue(), out);
}
- TopologySerializers.topology.serialize(from.local, out, version);
- TopologySerializers.topology.serialize(from.global, out, version);
+ TopologySerializers.topology.serialize(from.local, out);
+ TopologySerializers.topology.serialize(from.global, out);
}
@Override
- public Journal.TopologyUpdate deserialize(DataInputPlus in, int version) throws IOException
+ public Journal.TopologyUpdate deserialize(DataInputPlus in) throws IOException
{
int commandStoresSize = in.readUnsignedVInt32();
Int2ObjectHashMap<CommandStores.RangesForEpoch> commandStores = new Int2ObjectHashMap<>();
for (int j = 0; j < commandStoresSize; j++)
{
int commandStoreId = in.readUnsignedVInt32();
- CommandStores.RangesForEpoch rangesForEpoch = RangesForEpochSerializer.instance.deserialize(in, version);
+ CommandStores.RangesForEpoch rangesForEpoch = RangesForEpochSerializer.instance.deserialize(in);
commandStores.put(commandStoreId, rangesForEpoch);
}
- Topology local = TopologySerializers.topology.deserialize(in, version);
- Topology global = TopologySerializers.topology.deserialize(in, version);
+ Topology local = TopologySerializers.topology.deserialize(in);
+ Topology global = TopologySerializers.topology.deserialize(in);
return new Journal.TopologyUpdate(commandStores, local, global);
}
@Override
- public long serializedSize(Journal.TopologyUpdate from, int version)
+ public long serializedSize(Journal.TopologyUpdate from)
{
long size = TypeSizes.sizeofUnsignedVInt(from.commandStores.size());
for (Map.Entry<Integer, CommandStores.RangesForEpoch> e : from.commandStores.entrySet())
{
size += TypeSizes.sizeofUnsignedVInt(e.getKey());
- size += RangesForEpochSerializer.instance.serializedSize(e.getValue(), version);
+ size += RangesForEpochSerializer.instance.serializedSize(e.getValue());
}
- size += TopologySerializers.topology.serializedSize(from.local, version);
- size += TopologySerializers.topology.serializedSize(from.global, version);
+ size += TopologySerializers.topology.serializedSize(from.local);
+ size += TopologySerializers.topology.serializedSize(from.global);
return size;
}
}
- class Serializer implements IVersionedSerializer<AccordTopologyUpdate>
+ class Serializer implements org.apache.cassandra.io.Serializer<AccordTopologyUpdate>
{
public static Serializer instance = new Serializer();
- public void serialize(AccordTopologyUpdate t, DataOutputPlus out, int version) throws IOException
+ @Override
+ public void serialize(AccordTopologyUpdate t, DataOutputPlus out) throws IOException
{
out.writeUnsignedVInt(t.epoch());
out.writeUnsignedVInt32(t.kind().ordinal());
switch (t.kind())
{
case NewTopology:
- TopologyUpdateSerializer.instance.serialize(((NewTopology) t).update, out, version);
+ TopologyUpdateSerializer.instance.serialize(((NewTopology) t).update, out);
break;
case Topologies:
TopologyImage image = (TopologyImage) t;
out.writeBoolean(image.update != null);
if (image.update != null)
- TopologyUpdateSerializer.instance.serialize(image.update, out, version);
+ TopologyUpdateSerializer.instance.serialize(image.update, out);
if (image.syncStatus == null)
out.writeByte(Byte.MAX_VALUE);
else
out.writeByte(image.syncStatus.ordinal());
- KeySerializers.ranges.serialize(image.closed, out, version);
- KeySerializers.ranges.serialize(image.retired, out, version);
+ KeySerializers.ranges.serialize(image.closed, out);
+ KeySerializers.ranges.serialize(image.retired, out);
break;
default:
throw new UnhandledEnum(t.kind());
}
}
- public AccordTopologyUpdate deserialize(DataInputPlus in, int version) throws IOException
+ @Override
+ public AccordTopologyUpdate deserialize(DataInputPlus in) throws IOException
{
- int epoch = in.readUnsignedVInt32();
+ long epoch = in.readUnsignedVInt();
Review Comment:
bug, we read an epoch > int max which then failed in the tests. We write long, sizeOf is long, yet read int?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#issuecomment-2722366000
merged https://github.com/apache/cassandra/commit/29dc5a433ec637c7f23a6bdd85f0876f25a6fe93
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1985883898
##########
test/simulator/test/org/apache/cassandra/simulator/test/ShortAccordSimulationTest.java:
##########
@@ -24,6 +24,74 @@
import org.apache.cassandra.simulator.paxos.AccordSimulationRunner;
+/**
Review Comment:
this was unrelated. I found a failing seed so wanted to run it, and i find this documentation critical to run the tests... so added it
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1992460791
##########
test/unit/org/apache/cassandra/service/accord/serializers/EncodeAsVInt32Test.java:
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.service.accord.serializers;
+
+import org.junit.Test;
+
+import accord.utils.Gen;
+import accord.utils.Gens;
+import org.apache.cassandra.io.Serializers;
+import org.apache.cassandra.io.util.DataOutputBuffer;
+
+import static accord.utils.Property.qt;
+
+public class EncodeAsVInt32Test
+{
+ private static final Gen.IntGen ENUM_RANGE = Gens.ints().between(0, Integer.MAX_VALUE - 1);
+
+ @Test
+ public void withNulls()
+ {
+ @SuppressWarnings({ "resource", "IOResourceOpenedButNotSafelyClosed" }) DataOutputBuffer output = new DataOutputBuffer();
+ EncodeAsVInt32<Integer> serializer = EncodeAsVInt32.withNulls(Integer::intValue, Integer::valueOf);
Review Comment:
this type actually has a logical conflict bug, which is why i limited to `ENUM_RANGE`. When the encoded value is `-1` we produce `0` which is `null`. Based off the current usage this isn't a problem
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1985835132
##########
src/java/org/apache/cassandra/service/accord/AccordSerializers.java:
##########
@@ -232,41 +201,45 @@ public Clustering<?> deserialize(DataInputPlus in, int version) throws IOExcepti
}
@Override
- public long serializedSize(Clustering<?> clustering, int version)
+ public long serializedSize(Clustering<?> clustering)
{
return computeSerializedSize(clustering);
}
private <V> long computeSerializedSize(Clustering<V> clustering)
{
- int size = sizeof(true) + sizeofUnsignedVInt(clustering.size());
Review Comment:
this was a bug found by this refactor. When we "embed" the bytes we were not validating size == written like we do in networking; when you add that validation you found this was failing
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1982394448
##########
src/java/org/apache/cassandra/service/accord/AccordCache.java:
##########
@@ -1212,7 +1212,7 @@ public Object fullShrink(TxnId txnId, Command value)
try
{
- return AccordJournal.asSerializedChange(null, value, current_version);
+ return AccordJournal.asSerializedChange(null, value, Version.LATEST);
Review Comment:
TODO: is this in-memeory or a disk cache? if disk we shouldn't use `latest` but instead a downgrade safe version.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1982388348
##########
src/java/org/apache/cassandra/io/IVersionedSerializer.java:
##########
@@ -17,6 +17,34 @@
*/
package org.apache.cassandra.io;
+import java.io.IOException;
+
+import org.apache.cassandra.io.util.DataInputPlus;
+import org.apache.cassandra.io.util.DataOutputPlus;
+
public interface IVersionedSerializer<T> extends IVersionedAsymmetricSerializer<T, T>
{
+ static <T> IVersionedSerializer<T> fromSerializer(Serializer<T> delegate)
Review Comment:
TODO: this API isn't consistent, should look closer to make sure the APIs match for each interface
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell closed pull request #3954: CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin
URL: https://github.com/apache/cassandra/pull/3954
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1992434689
##########
src/java/org/apache/cassandra/service/accord/api/TokenKey.java:
##########
@@ -50,11 +50,6 @@
public final class TokenKey extends AccordRoutableKey implements RoutingKey, RangeFactory
{
- public enum RoutingKeyKind
Review Comment:
was only used by tests, so moved to tests
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "ifesdjeen (via GitHub)" <gi...@apache.org>.
ifesdjeen commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1994023160
##########
src/java/org/apache/cassandra/index/accord/RouteIndexFormat.java:
##########
@@ -178,7 +167,7 @@ public static ByteBuffer extractParticipants(RouteJournalIndex index, TxnId txnI
}
}
- public static AccordJournal.Builder extract(TxnId txnId, ByteBuffer record, int user_version)
+ public static AccordJournal.Builder extract(TxnId txnId, ByteBuffer record, Version user_version)
Review Comment:
nit; pre-existing: Should we switch user_version to `userVersion` (i.e. camelCase)?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1985435464
##########
src/java/org/apache/cassandra/net/MessagingService.java:
##########
@@ -224,23 +224,6 @@ public enum Version
VERSION_50(13),
VERSION_51(14);
- public static final Version CURRENT;
Review Comment:
documented why i removed this in `current()`, but the jist is that this is now touched by `Config` so touching DD adds a circular dependecy
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1982398315
##########
src/java/org/apache/cassandra/service/accord/AccordSerializers.java:
##########
@@ -54,23 +57,24 @@
import static org.apache.cassandra.db.marshal.CollectionType.Kind.LIST;
import static org.apache.cassandra.db.marshal.CollectionType.Kind.MAP;
import static org.apache.cassandra.db.marshal.CollectionType.Kind.SET;
+import static org.apache.cassandra.utils.ByteBufferUtil.nullableByteBufferSerializer;
public class AccordSerializers
{
+ public static <T> EmbeddedAsymmetricVersionedSerializer<T, T, Version> embedded(VersionedSerializer<T, Version> serializer)
Review Comment:
TODO: remove these methods, they are kinda unsafe as we odn't know the best version to use at this layer
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1985852393
##########
src/java/org/apache/cassandra/service/accord/serializers/Version.java:
##########
@@ -0,0 +1,133 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.service.accord.serializers;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+import org.apache.cassandra.db.TypeSizes;
+import org.apache.cassandra.io.MessageVersionProvider;
+import org.apache.cassandra.io.util.DataInputPlus;
+import org.apache.cassandra.io.util.DataOutputPlus;
+import org.apache.cassandra.net.MessagingService;
+
+public enum Version implements MessageVersionProvider
+{
+ // If MessagingService version bumps, this mapping does not need to be updated; only updates needed are those that
+ // include accord serializer changes.
+ V1(1, MessagingService.Version.VERSION_51);
+
+ public static final Version LATEST = Version.V1;
+ /**
+ * Version that should be used for disk serialization where downgrade may be possible.
+ *
+ * As of this writing only 1 version exists, so this is the same as LATEST... Once v2 comes into the picture we need this version to be the oldest version needed for downgrade... If you upgrade from 5.1 to 5.2 (assuming this adds a v2) you need a version that works with 5.1 here.
+ */
+ public static final Version DOWNGRADE_SAFE_VERSION = Version.V1;
+
+ /**
+ * Version that should be used for messaging serialization where mixed versions may be possible.
+ *
+ * As of this writing only 1 version exists, so this is the same as LATEST... Once v2 comes into the picture we need this version to be the oldest version needed for downgrade... If you upgrade from 5.1 to 5.2 (assuming this adds a v2) you need a version that works with 5.1 here.
+ */
+ public static final Version CLUSTER_SAFE_VERSION = Version.V1;
Review Comment:
these fields are here mostly for documentation at the moment as we only have one version.
One issue with `CLUSTER_SAFE_VERSION` is knowing what is safe. "should" this be part of the "capabilities" work some people want? Is this inferred based off version (how repair does this)? I left the answer to this out of scope for the moment.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1992435465
##########
src/java/org/apache/cassandra/service/accord/api/TokenKey.java:
##########
@@ -138,7 +135,7 @@ public Object printableSuffix()
if (isSentinel())
{
if (isTableSentinel()) suffix = isMin() ? "-Inf" : "+Inf";
- else suffix = suffix + "(-epsilon)";
+ if (isTokenSentinel()) suffix = (isBefore() ? "before(" : "after(") + suffix + ')';
Review Comment:
in C* we use `min(%s)`, i could do the same as we use `-Inf` for "min"
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1992456331
##########
test/unit/org/apache/cassandra/service/accord/journal/AccordTopologyUpdateTest.java:
##########
@@ -0,0 +1,175 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.service.accord.journal;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import accord.api.Journal;
+import accord.local.CommandStores;
+import accord.local.Node;
+import accord.primitives.Ranges;
+import accord.topology.Topology;
+import accord.utils.AccordGens;
+import accord.utils.Gen;
+import accord.utils.Gens;
+import org.agrona.collections.Int2ObjectHashMap;
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.dht.IPartitioner;
+import org.apache.cassandra.dht.Murmur3Partitioner;
+import org.apache.cassandra.io.Serializers;
+import org.apache.cassandra.io.util.DataOutputBuffer;
+import org.apache.cassandra.schema.TableId;
+import org.apache.cassandra.utils.AccordGenerators;
+
+import static accord.utils.Property.qt;
+
+public class AccordTopologyUpdateTest
+{
+ private static final long[] EPOCHS = new long[0];
+ private static final Ranges[] RANGES = new Ranges[0];
+ private static final TableId TBL1 = TableId.fromRaw(0, 0);
+
+ static
+ {
+ DatabaseDescriptor.clientInitialization();
+ }
+
+ @Before
+ public void before()
+ {
+ DatabaseDescriptor.setPartitionerUnsafe(Murmur3Partitioner.instance);
+ }
+
+ @Test
+ public void rangesForEpoch()
+ {
+ @SuppressWarnings({ "resource", "IOResourceOpenedButNotSafelyClosed" }) DataOutputBuffer output = new DataOutputBuffer();
+ qt().forAll(rangesForEpochGen()).check(expected -> {
+ maybeUpdatePartitioner(expected);
+ Serializers.testSerde(output, AccordTopologyUpdate.RangesForEpochSerializer.instance, expected);
+ });
+ }
+
+ @Test
+ public void topologyUpdate()
+ {
+ @SuppressWarnings({ "resource", "IOResourceOpenedButNotSafelyClosed" }) DataOutputBuffer output = new DataOutputBuffer();
+ qt().forAll(topologyUpdateGen()).check(expected -> {
+ maybeUpdatePartitioner(expected);
+ Serializers.testSerde(output, AccordTopologyUpdate.TopologyUpdateSerializer.instance, expected);
+ });
+ }
+
+ @Test
+ public void accordTopologyUpdate()
+ {
+ @SuppressWarnings({ "resource", "IOResourceOpenedButNotSafelyClosed" }) DataOutputBuffer output = new DataOutputBuffer();
+ qt().forAll(accordTopologyUpdateGen()).check(expected -> {
+ maybeUpdatePartitioner(expected);
+ Serializers.testSerde(output, AccordTopologyUpdate.Serializer.instance, expected);
+ });
+ }
+
+ private static Gen<CommandStores.RangesForEpoch> rangesForEpochGen()
+ {
+ return AccordGenerators.partitioner().flatMap(p -> rangesForEpochGen(AccordGenerators.rangesSplitOrArbitrary(p)));
+ }
+
+ private static Gen<CommandStores.RangesForEpoch> rangesForEpochGen(Gen<Ranges> rangesGen)
+ {
+ Gen.IntGen sizeGen = Gens.ints().between(0, 10);
+ Gen.LongGen epochGen = AccordGens.epochs();
+ return rs -> {
+ int size = sizeGen.nextInt(rs);
+ if (size == 0)
+ return new CommandStores.RangesForEpoch(EPOCHS, RANGES);
+ long epoch = epochGen.nextLong(rs);
+ long[] epochs = new long[size];
+ Ranges[] ranges = new Ranges[size];
+ for (int i = 0; i < size; i++)
+ {
+ epochs[i] = epoch++;
+ ranges[i] = rangesGen.next(rs);
+ }
+ return new CommandStores.RangesForEpoch(epochs, ranges);
+ };
+ }
+
+ private static Gen<Journal.TopologyUpdate> topologyUpdateGen()
+ {
+ Gen<IPartitioner> partitionerGen = AccordGenerators.partitioner();
+ return rs -> {
+ IPartitioner partitioner = partitionerGen.next(rs);
+ Gen<Ranges> rangesGen = AccordGenerators.ranges(TBL1, partitioner);
Review Comment:
limit to a single table as the perf of this generator isn't the best, we spend a lot of time creating ranges
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1985446586
##########
src/java/org/apache/cassandra/service/accord/interop/AccordInteropReadRepair.java:
##########
@@ -111,11 +112,11 @@ Object convertResponse(ReadOk ok)
private static final IVersionedSerializer<Data> noop_data_serializer = new IVersionedSerializer<>()
{
@Override
- public void serialize(Data t, DataOutputPlus out, int version) throws IOException {}
+ public void serialize(Data t, DataOutputPlus out, Version version) throws IOException {}
Review Comment:
even though this could be unversioned, the usage of it uses the same class for replies and that does have a version, so making this unversioned would complicate `ReadDataSerializers.ReplySerializer`
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1985836683
##########
src/java/org/apache/cassandra/service/accord/serializers/EnumSerializer.java:
##########
@@ -46,28 +44,19 @@ public E forOrdinal(int ordinal)
}
@Override
- public void serialize(E t, DataOutputPlus out, int version) throws IOException
+ public void serialize(E t, DataOutputPlus out) throws IOException
{
out.writeUnsignedVInt32(t.ordinal());
}
@Override
- public E deserialize(DataInputPlus in, int version) throws IOException
+ public E deserialize(DataInputPlus in) throws IOException
{
return values[in.readUnsignedVInt32()];
}
- public ByteBuffer serialize(E e)
Review Comment:
the base interface now has this method, so all serializers get this by default
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#issuecomment-2722018214
> explicit MessagingSerializer in place of IVersionedSerializer
Caleb is working on the rebase and this would be very problematic for him, so prefer to do this post merge to trunk
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1992436550
##########
src/java/org/apache/cassandra/service/accord/journal/AccordTopologyUpdate.java:
##########
@@ -55,178 +56,181 @@ static AccordTopologyUpdate newTopology(Journal.TopologyUpdate update)
{
return new NewTopology(update);
}
- class RangesForEpochSerializer implements IVersionedSerializer<CommandStores.RangesForEpoch>
+ class RangesForEpochSerializer implements org.apache.cassandra.io.Serializer<CommandStores.RangesForEpoch>
{
public static final RangesForEpochSerializer instance = new RangesForEpochSerializer();
@Override
- public void serialize(CommandStores.RangesForEpoch from, DataOutputPlus out, int version) throws IOException
+ public void serialize(CommandStores.RangesForEpoch from, DataOutputPlus out) throws IOException
{
out.writeUnsignedVInt32(from.size());
- from.forEach((epoch, ranges) -> {
- try
- {
- out.writeLong(epoch);
- KeySerializers.ranges.serialize(ranges, out, version);
- }
- catch (Throwable t)
- {
- throw new IllegalStateException("Serialization error", t);
- }
- });
+ for (int i = 0; i < from.size(); i++)
+ {
+ out.writeLong(from.epochAtIndex(i));
+ KeySerializers.ranges.serialize(from.rangesAtIndex(i), out);
+ }
}
@Override
- public CommandStores.RangesForEpoch deserialize(DataInputPlus in, int version) throws IOException
+ public CommandStores.RangesForEpoch deserialize(DataInputPlus in) throws IOException
{
int size = in.readUnsignedVInt32();
Ranges[] ranges = new Ranges[size];
long[] epochs = new long[size];
for (int i = 0; i < ranges.length; i++)
{
epochs[i] = in.readLong();
- ranges[i] = KeySerializers.ranges.deserialize(in, version);
+ ranges[i] = KeySerializers.ranges.deserialize(in);
}
Invariants.require(ranges.length == epochs.length);
return new CommandStores.RangesForEpoch(epochs, ranges);
}
@Override
- public long serializedSize(CommandStores.RangesForEpoch t, int version)
+ public long serializedSize(CommandStores.RangesForEpoch from)
{
- return TypeSizes.sizeofUnsignedVInt(t.size());
+ long size = TypeSizes.sizeofUnsignedVInt(from.size());
Review Comment:
this was a bug, didn't account for the epoch ranges
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1985883541
##########
test/distributed/org/apache/cassandra/distributed/impl/Instance.java:
##########
@@ -395,12 +395,14 @@ protected void registerOutboundFilter(ICluster cluster)
MessagingService.instance().outboundSink.add((message, to) -> {
if (isShutdown())
return false; // TODO: Simulator needs this to trigger a failure
- IMessage serialzied = serializeMessage(message.from(), to, message);
int fromNum = config.num(); // since this instance is sending the message, from will always be this instance
IInstance toInstance = cluster.get(fromCassandraInetAddressAndPort(to));
if (toInstance == null)
return true; // TODO: Simulator needs this to trigger a failure
+ if (!cluster.filters().hasInbound() && !cluster.filters().hasOutbound())
Review Comment:
perf issue, tests would have no filtering yet we would spend a ton of time deserializing to check.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1985887388
##########
test/unit/org/apache/cassandra/service/accord/txn/TxnConditionTest.java:
##########
@@ -0,0 +1,173 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.service.accord.txn;
+
+import java.nio.ByteBuffer;
+import java.nio.charset.CharacterCodingException;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import javax.annotation.Nullable;
+
+import org.junit.Test;
+
+import accord.utils.Gen;
+import accord.utils.Gens;
+import org.apache.cassandra.cql3.conditions.ColumnCondition;
+import org.apache.cassandra.cql3.conditions.ColumnConditionTest;
+import org.apache.cassandra.db.Clustering;
+import org.apache.cassandra.db.rows.CellPath;
+import org.apache.cassandra.io.Serializers;
+import org.apache.cassandra.io.util.DataOutputBuffer;
+import org.apache.cassandra.schema.ColumnMetadata;
+import org.apache.cassandra.schema.MockSchema;
+import org.apache.cassandra.schema.Schema;
+import org.apache.cassandra.service.accord.serializers.Version;
+import org.apache.cassandra.transport.ProtocolVersion;
+import org.apache.cassandra.utils.ByteBufferUtil;
+import org.apache.cassandra.utils.CassandraGenerators;
+import org.apache.cassandra.utils.Generators;
+
+import static accord.utils.Property.qt;
+
+//TOOD (maintaince): rather than copy the condition supported kinds, maybe references directly from the type?
+public class TxnConditionTest
Review Comment:
added this class as an issue was found. The issue ended up being clustering serialization of statics, but this test was useful to help make sure the changes were safe
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1985884288
##########
test/unit/org/apache/cassandra/cql3/conditions/ColumnConditionTest.java:
##########
@@ -930,11 +948,11 @@ private static org.quicktheories.core.Gen<AbstractType<?>> selectTypes(ColumnCon
}
}
- private static Gen<ColumnCondition.Bound> boundGen()
+ public static Gen<ColumnCondition.Bound> boundGen()
{
Gen<ColumnCondition.BoundKind> kindGen = Gens.enums().all(ColumnCondition.BoundKind.class);
Gen<Operator> operatorGen = Gens.enums().all(Operator.class);
- Gen<ByteBuffer> nonNullValuesGen = Generators.toGen(Generators.bytes(1, 100));
+ Gen<ByteBuffer> nonNullValuesGen = Generators.toGen(Generators.directAndHeapBytes(1, 100));
Review Comment:
this doesn't do the read only buffer types, those lead to jvm crashes
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1992444373
##########
src/java/org/apache/cassandra/service/accord/serializers/TopologySerializers.java:
##########
@@ -101,96 +77,85 @@ public <V> Node.Id deserialize(ByteBuffer src, int position)
return new Node.Id(src.getInt(position));
}
- public int serializedSize()
- {
- return TypeSizes.INT_SIZE; // id.id
- }
-
@Override
- public long serializedSize(Node.Id id, int version)
+ public long serializedSize(Node.Id id)
{
- return serializedSize();
- }
-
- @Override
- public long serializedSize(Node.Id t, Version version)
- {
- return serializedSize();
+ return TypeSizes.INT_SIZE; // id.id
}
- };
+ }
- public static final IVersionedSerializer<Shard> shard = new ShardSerializer((IVersionedSerializer<Range>)
- (IVersionedSerializer<?>)
- TokenRange.serializer);
+ public static final Serializer<Shard> shard = new ShardSerializer((Serializer<Range>)
+ (Serializer<?>)
+ TokenRange.serializer);
- public static class ShardSerializer implements IVersionedSerializer<Shard>
+ public static class ShardSerializer implements Serializer<Shard>
{
- protected IVersionedSerializer<Range> range;
+ protected Serializer<Range> range;
- public ShardSerializer(IVersionedSerializer<Range> range)
+ public ShardSerializer(Serializer<Range> range)
{
this.range = range;
}
@Override
- public void serialize(Shard shard, DataOutputPlus out, int version) throws IOException
+ public void serialize(Shard shard, DataOutputPlus out) throws IOException
{
- range.serialize(shard.range, out, version);
- CollectionSerializers.serializeList(shard.nodes, out, version, nodeId);
- CollectionSerializers.serializeList(shard.notInFastPath, out, version, nodeId);
- CollectionSerializers.serializeList(shard.joining, out, version, nodeId);
+ range.serialize(shard.range, out);
+ CollectionSerializers.serializeList(shard.nodes, out, nodeId);
+ CollectionSerializers.serializeList(shard.notInFastPath, out, nodeId);
+ CollectionSerializers.serializeList(shard.joining, out, nodeId);
out.writeUnsignedVInt32(shard.flags().bitset());
}
@Override
- public Shard deserialize(DataInputPlus in, int version) throws IOException
+ public Shard deserialize(DataInputPlus in) throws IOException
{
- Range range = ShardSerializer.this.range.deserialize(in, version);
- SortedArrayList<Node.Id> nodes = CollectionSerializers.deserializeSortedArrayList(in, version, nodeId, Node.Id[]::new);
- SortedArrayList<Node.Id> notInFastPath = CollectionSerializers.deserializeSortedArrayList(in, version, nodeId, Node.Id[]::new);
- SortedArrayList<Node.Id> joining = CollectionSerializers.deserializeSortedArrayList(in, version, nodeId, Node.Id[]::new);
+ Range range = ShardSerializer.this.range.deserialize(in);
+ SortedArrayList<Node.Id> nodes = CollectionSerializers.deserializeSortedArrayList(in, nodeId, Node.Id[]::new);
+ SortedArrayList<Node.Id> notInFastPath = CollectionSerializers.deserializeSortedArrayList(in, nodeId, Node.Id[]::new);
+ SortedArrayList<Node.Id> joining = CollectionSerializers.deserializeSortedArrayList(in, nodeId, Node.Id[]::new);
int flags = in.readUnsignedVInt32();
return Shard.SerializerSupport.create(range, nodes, notInFastPath, joining, new TinyEnumSet<>(flags));
}
@Override
- public long serializedSize(Shard shard, int version)
+ public long serializedSize(Shard shard)
{
- long size = range.serializedSize(shard.range, version);
- size += CollectionSerializers.serializedListSize(shard.nodes, version, nodeId);
- size += CollectionSerializers.serializedListSize(shard.notInFastPath, version, nodeId);
- size += CollectionSerializers.serializedListSize(shard.joining, version, nodeId);
+ long size = range.serializedSize(shard.range);
+ size += CollectionSerializers.serializedListSize(shard.nodes, nodeId);
+ size += CollectionSerializers.serializedListSize(shard.notInFastPath, nodeId);
+ size += CollectionSerializers.serializedListSize(shard.joining, nodeId);
size += TypeSizes.sizeofUnsignedVInt(shard.flags().bitset());
return size;
}
- };
+ }
- public static final IVersionedSerializer<Topology> topology = new IVersionedSerializer<>()
+ public static final Serializer<Topology> topology = new Serializer<>()
{
@Override
- public void serialize(Topology topology, DataOutputPlus out, int version) throws IOException
+ public void serialize(Topology topology, DataOutputPlus out) throws IOException
{
out.writeLong(topology.epoch());
- ArraySerializers.serializeArray(topology.unsafeGetShards(), out, version, shard);
Review Comment:
`unsafeGetShards` is unsafe and causes this to fail. we have a `local` and `global` topology we serialize, but the `local` topology comes back as `global` when we deserialize! moving to `shards` makes sure we respect this.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "iamaleksey (via GitHub)" <gi...@apache.org>.
iamaleksey commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1993799678
##########
src/java/org/apache/cassandra/utils/UUIDSerializer.java:
##########
@@ -35,13 +36,32 @@ public void serialize(UUID uuid, DataOutputPlus out, int version) throws IOExcep
out.writeLong(uuid.getLeastSignificantBits());
}
+ @Override
+ public void serialize(UUID uuid, DataOutputPlus out) throws IOException
+ {
+ out.writeLong(uuid.getMostSignificantBits());
+ out.writeLong(uuid.getLeastSignificantBits());
+ }
+
public UUID deserialize(DataInputPlus in, int version) throws IOException
Review Comment:
Can delegate to unversioned calls instead of duplicating? But ideally just no longer implementing `IVersionedSerializer` (maybe in a follow-up commit after Accord's been merged).
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1992455597
##########
test/unit/org/apache/cassandra/service/accord/AccordConfigurationServiceTest.java:
##########
@@ -218,7 +218,8 @@ public AsyncResult<Void> onTopologyUpdate(Topology topology, boolean isLoad, boo
}
finally
{
- journal.shutdown();
+ if (journal != null)
Review Comment:
not directly a bug fix... when i broke one serializer the error was lost due to this NPE. So made it so we don't NPE and get the original error
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1993966311
##########
src/java/org/apache/cassandra/utils/UUIDSerializer.java:
##########
@@ -35,13 +36,32 @@ public void serialize(UUID uuid, DataOutputPlus out, int version) throws IOExcep
out.writeLong(uuid.getLeastSignificantBits());
}
+ @Override
+ public void serialize(UUID uuid, DataOutputPlus out) throws IOException
+ {
+ out.writeLong(uuid.getMostSignificantBits());
+ out.writeLong(uuid.getLeastSignificantBits());
+ }
+
public UUID deserialize(DataInputPlus in, int version) throws IOException
Review Comment:
i made sure to add `@Overrride` so this method doesn't leak once we actually migrate off IVersionedSerializer.
I moved all version methods to call the unversioned methods, so there isn't duplicate now
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1985887660
##########
test/unit/org/apache/cassandra/utils/CollectionSerializersTest.java:
##########
@@ -0,0 +1,123 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.utils;
+
+import java.io.IOException;
+
+import org.junit.Test;
+
+import accord.utils.Gens;
+import org.apache.cassandra.db.TypeSizes;
+import org.apache.cassandra.io.IVersionedSerializer;
+import org.apache.cassandra.io.Serializer;
+import org.apache.cassandra.io.VersionedSerializer;
+import org.apache.cassandra.io.util.DataInputPlus;
+import org.apache.cassandra.io.util.DataOutputBuffer;
+import org.apache.cassandra.io.util.DataOutputPlus;
+
+import static accord.utils.Property.qt;
+import static org.apache.cassandra.io.Serializers.testSerde;
+import static org.apache.cassandra.utils.CollectionSerializers.newListSerializer;
+
+public class CollectionSerializersTest
Review Comment:
i hit an issue and it wasn't clear if collections were the root cause so created this test. It turned out that static clustering was the root cause, but left this as it improved coverage
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1985453147
##########
src/java/org/apache/cassandra/service/accord/serializers/AsymmetricVersionedSerializer.java:
##########
@@ -0,0 +1,23 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.service.accord.serializers;
+
+public interface AsymmetricVersionedSerializer<In, Out> extends org.apache.cassandra.io.AsymmetricVersionedSerializer<In, Out, Version>
Review Comment:
this exists to simplify the migration by making it a import swap. The question of "should" this exist mostly should ask if this short hand is useful, as most users should export the type `org.apache.cassandra.io.AsymmetricVersionedSerializer`
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1985853888
##########
src/java/org/apache/cassandra/service/accord/txn/AbstractSerialized.java:
##########
@@ -84,8 +109,42 @@ public void unmemoize()
memoized = null;
}
- protected ByteBuffer bytes()
+ @Nullable
+ protected ByteBuffer unsafeBytes()
{
return bytes;
}
+
+ @Nonnull
+ protected ByteBuffer bytes(Version target)
+ {
+ Invariants.nonNull(bytes);
Review Comment:
its nullable in a few contexts, but not in any context that *needs* the bytes, so this is here to enforce that behavior.
##########
src/java/org/apache/cassandra/service/accord/txn/AbstractSerialized.java:
##########
@@ -84,8 +109,42 @@ public void unmemoize()
memoized = null;
}
- protected ByteBuffer bytes()
+ @Nullable
+ protected ByteBuffer unsafeBytes()
{
return bytes;
}
+
+ @Nonnull
+ protected ByteBuffer bytes(Version target)
+ {
+ Invariants.nonNull(bytes);
Review Comment:
its nullable in one contexts, but not in any context that *needs* the bytes, so this is here to enforce that behavior.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1985857166
##########
src/java/org/apache/cassandra/service/accord/txn/TxnUpdate.java:
##########
@@ -339,7 +339,7 @@ private static <T> List<T> deserialize(ByteBuffer bytes, IVersionedSerializer<T>
try (DataInputBuffer in = new DataInputBuffer(bytes, true))
{
- int version = in.readUnsignedVInt32();
+ Version version = Version.fromVersion(in.readUnsignedVInt32());
Review Comment:
punted on fixing in this patch as its kinda annoying, so can try to fix after the other open issues are addressed?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1985884510
##########
test/unit/org/apache/cassandra/io/IVersionedSerializers.java:
##########
@@ -1,41 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-package org.apache.cassandra.io;
-
-import java.io.IOException;
-
-import accord.utils.LazyToString;
-import accord.utils.ReflectionUtils;
-import org.apache.cassandra.io.util.DataInputBuffer;
-import org.apache.cassandra.io.util.DataOutputBuffer;
-import org.assertj.core.api.Assertions;
-
-public class IVersionedSerializers
Review Comment:
this was renamed to `test/unit/org/apache/cassandra/io/Serializers.java`
##########
test/unit/org/apache/cassandra/io/Serializers.java:
##########
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.io;
+
+import java.io.IOException;
+
+import accord.utils.LazyToString;
+import accord.utils.ReflectionUtils;
+import org.apache.cassandra.io.util.DataInputBuffer;
+import org.apache.cassandra.io.util.DataOutputBuffer;
+import org.assertj.core.api.Assertions;
+
+public class Serializers
Review Comment:
this was renamed from `test/unit/org/apache/cassandra/io/IVersionedSerializers.java`
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1985885975
##########
test/unit/org/apache/cassandra/service/accord/AccordSerializersTest.java:
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.service.accord;
+
+import org.junit.Test;
+
+import org.apache.cassandra.io.Serializers;
+import org.apache.cassandra.io.util.DataOutputBuffer;
+import org.apache.cassandra.utils.CassandraGenerators;
+import org.apache.cassandra.utils.Generators;
+
+import static accord.utils.Property.qt;
+
+public class AccordSerializersTest
+{
+ @Test
+ public void clustering()
Review Comment:
there was a bug with statics, so created this test to help make sure stable
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1990401379
##########
src/java/org/apache/cassandra/service/accord/api/TokenKey.java:
##########
@@ -193,6 +180,20 @@ public RoutingKey toUnseekable()
return this;
}
+ public boolean isMin()
+ {
+ //TODO (review): some code paths don't care if before/after are used, but some are not fully correct (range.isFullRange)
+// return sentinel == MIN_TABLE_SENTINEL;
+ return (sentinel & PREFIX_MASK) == (MIN_TABLE_SENTINEL & PREFIX_MASK);
Review Comment:
`min().before().isMin() == false` without this patch, but it looks like most code paths don't want this behavior
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#discussion_r1992436320
##########
src/java/org/apache/cassandra/service/accord/journal/AccordTopologyUpdate.java:
##########
@@ -55,178 +56,181 @@ static AccordTopologyUpdate newTopology(Journal.TopologyUpdate update)
{
return new NewTopology(update);
}
- class RangesForEpochSerializer implements IVersionedSerializer<CommandStores.RangesForEpoch>
+ class RangesForEpochSerializer implements org.apache.cassandra.io.Serializer<CommandStores.RangesForEpoch>
{
public static final RangesForEpochSerializer instance = new RangesForEpochSerializer();
@Override
- public void serialize(CommandStores.RangesForEpoch from, DataOutputPlus out, int version) throws IOException
+ public void serialize(CommandStores.RangesForEpoch from, DataOutputPlus out) throws IOException
{
out.writeUnsignedVInt32(from.size());
- from.forEach((epoch, ranges) -> {
- try
- {
- out.writeLong(epoch);
- KeySerializers.ranges.serialize(ranges, out, version);
- }
- catch (Throwable t)
- {
- throw new IllegalStateException("Serialization error", t);
- }
- });
+ for (int i = 0; i < from.size(); i++)
Review Comment:
rewrote just to avoid the catch then rethrow, and is smaller
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20403: Decouple Accord serializers from C* message version so accord can make changes without impacting C* messagin [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on PR #3954:
URL: https://github.com/apache/cassandra/pull/3954#issuecomment-2722009548
renamed Serializer to UnversionedSerializer
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org