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/06/16 22:28:19 UTC
[PR] CASSANDRA-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
dcapwell opened a new pull request, #4202:
URL: https://github.com/apache/cassandra/pull/4202
(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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2191021211
##########
src/java/org/apache/cassandra/service/accord/serializers/TopologySerializers.java:
##########
@@ -159,4 +167,127 @@ public long serializedSize(Topology topology)
return size;
}
};
+
+ public static final UnversionedSerializer<Topology> compactTopology = new UnversionedSerializer<>()
+ {
+ @Override
+ public void serialize(Topology topology, DataOutputPlus out) throws IOException
+ {
+ out.writeLong(topology.epoch());
+ CollectionSerializers.serializeList(topology.staleIds(), out, TopologySerializers.nodeId);
+
+ List<Shard> shards = topology.shards();
+
+ // need to loop twice; once to collect tables/ranges, and another to save shards
+ ImmutableUniqueList<TableId> tables;
+ ImmutableUniqueList<TokenRange> ranges;
+ {
+ ImmutableUniqueList.Builder<TableId> tablesBuilder = ImmutableUniqueList.builder();
+ ImmutableUniqueList.Builder<TokenRange> rangesBuilder = ImmutableUniqueList.builder();
+ for (Shard shard : shards)
+ {
+ TokenRange range = (TokenRange) shard.range;
+ tablesBuilder.add(range.table());
+ rangesBuilder.add(range.withTable(TableId.UNDEFINED));
+ }
+ tables = tablesBuilder.buildAndClear();
+ ranges = rangesBuilder.buildAndClear();
+ }
+
+ CollectionSerializers.serializeList(tables, out, TableId.compactComparableSerializer);
+ CollectionSerializers.serializeList(ranges, out, TokenRange.noTableSerializer);
+
+ out.writeUnsignedVInt32(shards.size());
+ for (Shard shard : shards)
+ {
+ TokenRange range = (TokenRange) shard.range;
+ int tableIdx = tables.indexOf(range.table());
+ int rangeIdx = ranges.indexOf(range.withTable(TableId.UNDEFINED));
+ out.writeUnsignedVInt32(tableIdx);
+ out.writeUnsignedVInt32(rangeIdx);
+
+ CollectionSerializers.serializeList(shard.nodes, out, TopologySerializers.nodeId);
+ BitSet notInFastPath = SortedArrays.toBitSet(shard.nodes, shard.notInFastPath);
+ BitSetSerializer.instance.serialize(notInFastPath, out);
+ BitSet joining = SortedArrays.toBitSet(shard.nodes, shard.joining);
+ BitSetSerializer.instance.serialize(joining, out);
Review Comment:
sorry, i wasn't clear. This specific line isn't what i am talking about, but the node ids are. joining is 1 byte in the common case now which should be in-line with previous version. But when something is joining its now 2 bytes vs 5.
Same happens in the `notInFastPath`. This logic is the most repeated so got the largest savings
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2191027212
##########
src/java/org/apache/cassandra/service/accord/serializers/TopologySerializers.java:
##########
@@ -159,4 +167,127 @@ public long serializedSize(Topology topology)
return size;
}
};
+
+ public static final UnversionedSerializer<Topology> compactTopology = new UnversionedSerializer<>()
+ {
+ @Override
+ public void serialize(Topology topology, DataOutputPlus out) throws IOException
+ {
+ out.writeLong(topology.epoch());
+ CollectionSerializers.serializeList(topology.staleIds(), out, TopologySerializers.nodeId);
+
+ List<Shard> shards = topology.shards();
+
+ // need to loop twice; once to collect tables/ranges, and another to save shards
+ ImmutableUniqueList<TableId> tables;
Review Comment:
> why aren't we just using Object2IntHashMap directly?
Ill take a look at the issues that faces.
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2193448232
##########
src/java/org/apache/cassandra/service/accord/serializers/TopologySerializers.java:
##########
@@ -159,4 +167,127 @@ public long serializedSize(Topology topology)
return size;
}
};
+
+ public static final UnversionedSerializer<Topology> compactTopology = new UnversionedSerializer<>()
+ {
+ @Override
+ public void serialize(Topology topology, DataOutputPlus out) throws IOException
+ {
+ out.writeLong(topology.epoch());
+ CollectionSerializers.serializeList(topology.staleIds(), out, TopologySerializers.nodeId);
+
+ List<Shard> shards = topology.shards();
+
+ // need to loop twice; once to collect tables/ranges, and another to save shards
+ ImmutableUniqueList<TableId> tables;
Review Comment:
pushed the change
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2192756702
##########
src/java/org/apache/cassandra/service/accord/serializers/TopologySerializers.java:
##########
@@ -159,4 +167,127 @@ public long serializedSize(Topology topology)
return size;
}
};
+
+ public static final UnversionedSerializer<Topology> compactTopology = new UnversionedSerializer<>()
+ {
+ @Override
+ public void serialize(Topology topology, DataOutputPlus out) throws IOException
+ {
+ out.writeLong(topology.epoch());
+ CollectionSerializers.serializeList(topology.staleIds(), out, TopologySerializers.nodeId);
+
+ List<Shard> shards = topology.shards();
+
+ // need to loop twice; once to collect tables/ranges, and another to save shards
+ ImmutableUniqueList<TableId> tables;
Review Comment:
In fact, you can even do this dynamically - which is another common technique in our serializers. When you go to deserialise a value, you first deserialize the index, and if the index is some sentinel (e.g. 0), then you deserialize the value and insert it into the List; otherwise you lookup the relevant index that has already been deserialised. On serialization, when you lookup the index, if it isn't there you write the sentinel and then the value.
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2184904384
##########
src/java/org/apache/cassandra/service/accord/serializers/TopologySerializers.java:
##########
@@ -159,4 +167,127 @@ public long serializedSize(Topology topology)
return size;
}
};
+
+ public static final UnversionedSerializer<Topology> compactTopology = new UnversionedSerializer<>()
+ {
+ @Override
+ public void serialize(Topology topology, DataOutputPlus out) throws IOException
+ {
+ out.writeLong(topology.epoch());
+ CollectionSerializers.serializeList(topology.staleIds(), out, TopologySerializers.nodeId);
+
+ List<Shard> shards = topology.shards();
+
+ // need to loop twice; once to collect tables/ranges, and another to save shards
+ ImmutableUniqueList<TableId> tables;
Review Comment:
I don't see why this class is needed for these serializers - why aren't we just using `Object2IntHashMap` directly? It would be more efficient, require no new types, and be more obvious to a reader. If you want to introduce a helper method for both serialising the array/list, and building the lookup that would make sense to me.
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2194557820
##########
src/java/org/apache/cassandra/service/accord/serializers/TopologySerializers.java:
##########
@@ -159,4 +168,141 @@ public long serializedSize(Topology topology)
return size;
}
};
+
+ public static final UnversionedSerializer<Topology> compactTopology = new UnversionedSerializer<>()
+ {
+ @Override
+ public void serialize(Topology topology, DataOutputPlus out) throws IOException
+ {
+ out.writeLong(topology.epoch());
+ CollectionSerializers.serializeList(topology.staleIds(), out, TopologySerializers.nodeId);
+
+ List<Shard> shards = topology.shards();
+
+ // need to loop twice; once to collect tables/ranges, and another to save shards
+ Object2IntHashMap<TableId> tables;
+ Object2IntHashMap<TokenRange> ranges;
+ {
+ Object2IntHashMap<TableId> tablesBuilder = new Object2IntHashMap<>(-2);
Review Comment:
suggestion: for tableId at least, do we even really need to serialize as a separate collection and encode indexes? We should visit `Shard` in `TableId` order, so we could just always have one active `TableId` and simply encode a boolean indicating whether we need to deserialize a new one?
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2184907125
##########
src/java/org/apache/cassandra/service/accord/serializers/TopologySerializers.java:
##########
@@ -159,4 +167,127 @@ public long serializedSize(Topology topology)
return size;
}
};
+
+ public static final UnversionedSerializer<Topology> compactTopology = new UnversionedSerializer<>()
+ {
+ @Override
+ public void serialize(Topology topology, DataOutputPlus out) throws IOException
+ {
+ out.writeLong(topology.epoch());
+ CollectionSerializers.serializeList(topology.staleIds(), out, TopologySerializers.nodeId);
+
+ List<Shard> shards = topology.shards();
+
+ // need to loop twice; once to collect tables/ranges, and another to save shards
+ ImmutableUniqueList<TableId> tables;
+ ImmutableUniqueList<TokenRange> ranges;
+ {
+ ImmutableUniqueList.Builder<TableId> tablesBuilder = ImmutableUniqueList.builder();
+ ImmutableUniqueList.Builder<TokenRange> rangesBuilder = ImmutableUniqueList.builder();
+ for (Shard shard : shards)
+ {
+ TokenRange range = (TokenRange) shard.range;
+ tablesBuilder.add(range.table());
+ rangesBuilder.add(range.withTable(TableId.UNDEFINED));
+ }
+ tables = tablesBuilder.buildAndClear();
+ ranges = rangesBuilder.buildAndClear();
+ }
+
+ CollectionSerializers.serializeList(tables, out, TableId.compactComparableSerializer);
+ CollectionSerializers.serializeList(ranges, out, TokenRange.noTableSerializer);
+
+ out.writeUnsignedVInt32(shards.size());
+ for (Shard shard : shards)
+ {
+ TokenRange range = (TokenRange) shard.range;
+ int tableIdx = tables.indexOf(range.table());
+ int rangeIdx = ranges.indexOf(range.withTable(TableId.UNDEFINED));
+ out.writeUnsignedVInt32(tableIdx);
+ out.writeUnsignedVInt32(rangeIdx);
+
+ CollectionSerializers.serializeList(shard.nodes, out, TopologySerializers.nodeId);
+ BitSet notInFastPath = SortedArrays.toBitSet(shard.nodes, shard.notInFastPath);
+ BitSetSerializer.instance.serialize(notInFastPath, out);
Review Comment:
If we use accord's `SimpleBitSet` we can de/serialize without allocating a new byte[]
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2194525378
##########
src/java/org/apache/cassandra/utils/UniqueList.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * 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.util.List;
+import java.util.Set;
+import java.util.Spliterator;
+
+/**
+ * A collection that maintains both List and Set properties:
+ * - As a List: Preserves order and allows deterministic iteration
+ * - As a Set: Ensures all elements are unique
+ * <p>
+ * This interface combines the ordered access of List with the uniqueness constraint of Set.
+ */
+public interface UniqueList<T> extends List<T>, Set<T>
Review Comment:
can move this back?
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2193401089
##########
src/java/org/apache/cassandra/service/accord/serializers/TopologySerializers.java:
##########
@@ -159,4 +167,127 @@ public long serializedSize(Topology topology)
return size;
}
};
+
+ public static final UnversionedSerializer<Topology> compactTopology = new UnversionedSerializer<>()
+ {
+ @Override
+ public void serialize(Topology topology, DataOutputPlus out) throws IOException
+ {
+ out.writeLong(topology.epoch());
+ CollectionSerializers.serializeList(topology.staleIds(), out, TopologySerializers.nodeId);
+
+ List<Shard> shards = topology.shards();
+
+ // need to loop twice; once to collect tables/ranges, and another to save shards
+ ImmutableUniqueList<TableId> tables;
Review Comment:
There are some other variations on this, fwiw, if you want to serialise a size upfront:
*Option 1*
You can first build your lookup, then serialise in the correct order with a second loop
```
for (value : values)
indexLookup.putIfAbsent(value, indexLookup.size())
int count = 0;
for (value : values)
if (indexLookup.get(value) == count)
{
count++;
out.write(value);
}
```
*Option 2*
You can first build your lookup without indexes, then assign them indexes in the order of iteration for the collection. The index is arbitrary, so this is fine, and better if there are many duplicates.
```
for (value : values)
indexLookup.putIfAbsent(value, -1)
int count = 0;
for (entry : indexLookup.entrySet())
entry.setValue(count++)
```
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2191566496
##########
src/java/org/apache/cassandra/service/accord/serializers/TopologySerializers.java:
##########
@@ -159,4 +167,127 @@ public long serializedSize(Topology topology)
return size;
}
};
+
+ public static final UnversionedSerializer<Topology> compactTopology = new UnversionedSerializer<>()
+ {
+ @Override
+ public void serialize(Topology topology, DataOutputPlus out) throws IOException
+ {
+ out.writeLong(topology.epoch());
+ CollectionSerializers.serializeList(topology.staleIds(), out, TopologySerializers.nodeId);
+
+ List<Shard> shards = topology.shards();
+
+ // need to loop twice; once to collect tables/ranges, and another to save shards
+ ImmutableUniqueList<TableId> tables;
Review Comment:
It is new to src, which to me is new to the PR. The expectations of src are different, and we will need to at least haggle about naming etc. But, even if it were already in src it is not a suitable data structure here as it imposes unnecessary costs in at least one but likely both directions.
For deserialisation, the object->int mapping is unused, so this is a costly waste of effort to construct.
For serialisation I don’t see why you need to sort the values? The normal approach here is to simply write them in the order they come, and ensure they are inserted into the hash map in that same order, so that the hash map contains their index. This saves an array allocation.
That is, on deserialization you simply build the original array/list and lookup by index, and on serialisation you serialise the array/list and build a lookup for finding the index to encode. This pattern is used extensively throughout the code base. In both directions we are cheaper 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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2151004280
##########
src/java/org/apache/cassandra/service/accord/CompactTopology.java:
##########
@@ -0,0 +1,341 @@
+/*
+ * 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 java.io.IOException;
+import java.util.ArrayList;
+import java.util.BitSet;
+import java.util.List;
+import java.util.Objects;
+
+import accord.local.Node;
+import accord.topology.Shard;
+import accord.topology.Topology;
+import accord.utils.SortedArrays;
+import accord.utils.TinyEnumSet;
+import org.apache.cassandra.db.TypeSizes;
+import org.apache.cassandra.io.UnversionedSerializer;
+import org.apache.cassandra.io.util.DataInputPlus;
+import org.apache.cassandra.io.util.DataOutputPlus;
+import org.apache.cassandra.schema.TableId;
+import org.apache.cassandra.service.accord.api.TokenKey;
+import org.apache.cassandra.service.accord.serializers.TopologySerializers;
+import org.apache.cassandra.utils.BitSetSerializer;
+import org.apache.cassandra.utils.CollectionSerializers;
+import org.apache.cassandra.utils.ImmutableUniqueList;
+
+public class CompactTopology
Review Comment:
This is the core data structure for this patch. It dedups the following
1) `TableId`
2) `Range` (`TokenRange` without `TableId`)
3) `Node.Id`
In testing different topology shapes, the savings is between 40-70%, and maybe more with higher table/node counts
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2155070634
##########
src/java/org/apache/cassandra/utils/ImmutableUniqueList.java:
##########
@@ -107,6 +119,23 @@ public int size()
return values.length;
}
+ @Override
+ public boolean equals(Object o)
Review Comment:
also, this respects the fact that a set.equals(ImmutableUniqueList) would return `true`, so this set should also return `true`; forcing order means that we loose that property
```java
a.equals(b) == b.equals(a)
```
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#issuecomment-3070888770
merged in 8289bb4cdee20c59add34858b817640e7f88518e
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2198659800
##########
src/java/org/apache/cassandra/utils/UniqueList.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * 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.util.List;
+import java.util.Set;
+import java.util.Spliterator;
+
+/**
+ * A collection that maintains both List and Set properties:
+ * - As a List: Preserves order and allows deterministic iteration
+ * - As a Set: Ensures all elements are unique
+ * <p>
+ * This interface combines the ordered access of List with the uniqueness constraint of Set.
+ */
+public interface UniqueList<T> extends List<T>, Set<T>
Review Comment:
done
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2193417436
##########
src/java/org/apache/cassandra/service/accord/serializers/TopologySerializers.java:
##########
@@ -159,4 +167,127 @@ public long serializedSize(Topology topology)
return size;
}
};
+
+ public static final UnversionedSerializer<Topology> compactTopology = new UnversionedSerializer<>()
+ {
+ @Override
+ public void serialize(Topology topology, DataOutputPlus out) throws IOException
+ {
+ out.writeLong(topology.epoch());
+ CollectionSerializers.serializeList(topology.staleIds(), out, TopologySerializers.nodeId);
+
+ List<Shard> shards = topology.shards();
+
+ // need to loop twice; once to collect tables/ranges, and another to save shards
+ ImmutableUniqueList<TableId> tables;
Review Comment:
> For deserialisation, the object->int mapping is unused, so this is a costly waste of effort to construct.
Agree, on deserialize it could be `List` directly
> The normal approach here is to simply write them in the order they come, and ensure they are inserted into the hash map in that same order, so that the hash map contains their index. This saves an array allocation.
We then loose the header saying how many items exist, but you try to address this in the last comment.
Ill take a look at option 1/2 and play around with 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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2155087945
##########
test/unit/org/apache/cassandra/service/accord/FetchTopologiesTest.java:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.config.DatabaseDescriptor;
+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 FetchTopologiesTest
Review Comment:
tried looking at testing the full fetch topology state machine but was too much work for this patch, and its not the direct concern of this patch, so sticked with only testing the serializes impacted
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2155082336
##########
test/distributed/org/apache/cassandra/service/accord/AccordJournalBurnTest.java:
##########
@@ -115,6 +115,11 @@ public static void setUp() throws Throwable
FieldUtil.setInstanceUnsafe(TopologySerializers.class,
new TopologySerializers.ShardSerializer(BurnTestKeySerializers.range),
"shard");
+ // compact topology inlines all serialziation and uses TokenRange directly, so it has to be fully stubbed out
+ // for this class.
+ FieldUtil.setInstanceUnsafe(TopologySerializers.class,
Review Comment:
trying to make this flexible seemed a bit too much as i need to do a larger refactor over burn test and try to remove all our serialize overrides.
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2155084596
##########
test/harry/main/org/apache/cassandra/harry/model/ASTSingleTableModel.java:
##########
@@ -1539,9 +1539,10 @@ private List<PrimaryKey> findByPartitionEq(LookupContext ctx)
private Clustering<ByteBuffer> key(Map<Symbol, Expression> values, ImmutableUniqueList<Symbol> columns)
{
+ if (columns.isEmpty()) return Clustering.EMPTY;
Review Comment:
bug reported by @sunil9977 ; since i was already here quick to patch. The bug is that not every table has clustering columns so we should return empty.
This wasn't noticed as CI doesn't run harry unit 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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2155068178
##########
src/java/org/apache/cassandra/utils/ImmutableUniqueList.java:
##########
@@ -107,6 +119,23 @@ public int size()
return values.length;
}
+ @Override
+ public boolean equals(Object o)
Review Comment:
```java
jshell> var order = new LinkedHashSet<Integer>()
order ==> []
jshell> var unordered = new HashSet<Integer>()
unordered ==> []
jshell> order.add(4); order.add(3);
$3 ==> true
$4 ==> true
jshell> unordered.add(3); unordered.add(4);
$5 ==> true
$6 ==> true
jshell> order.equals(unordered)
$7 ==> true
jshell> unordered.equals(or
order
jshell> unordered.equals(order);
$8 ==> true
```
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2191566496
##########
src/java/org/apache/cassandra/service/accord/serializers/TopologySerializers.java:
##########
@@ -159,4 +167,127 @@ public long serializedSize(Topology topology)
return size;
}
};
+
+ public static final UnversionedSerializer<Topology> compactTopology = new UnversionedSerializer<>()
+ {
+ @Override
+ public void serialize(Topology topology, DataOutputPlus out) throws IOException
+ {
+ out.writeLong(topology.epoch());
+ CollectionSerializers.serializeList(topology.staleIds(), out, TopologySerializers.nodeId);
+
+ List<Shard> shards = topology.shards();
+
+ // need to loop twice; once to collect tables/ranges, and another to save shards
+ ImmutableUniqueList<TableId> tables;
Review Comment:
It is new to src, which to me is new to the PR. The expectations of src are different, and we will need to at least haggle about naming etc. But, even if it were already in src it is not a suitable data structure here as it imposes unnecessary costs in at least one but likely both directions.
For deserialisation, the object->int mapping is unused, so this is a costly waste of effort to construct.
For serialisation I don’t see why you need to sort the values? The normal approach here is to simply write them in the order they come, and ensure they are inserted into the hash map in that same order, so that the hash map contains their index. This saves an array allocation.
That is, on deserialization you simply build the original array/list and lookup by index, and on serialisation you serialise the array/list and build a lookup for finding the index to encode. This pattern is used extensively throughout the code base. In both directions we are cheaper this way.
Since it’s common, introducing dedicated serialisation methods for this pattern would make sense.
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2191566496
##########
src/java/org/apache/cassandra/service/accord/serializers/TopologySerializers.java:
##########
@@ -159,4 +167,127 @@ public long serializedSize(Topology topology)
return size;
}
};
+
+ public static final UnversionedSerializer<Topology> compactTopology = new UnversionedSerializer<>()
+ {
+ @Override
+ public void serialize(Topology topology, DataOutputPlus out) throws IOException
+ {
+ out.writeLong(topology.epoch());
+ CollectionSerializers.serializeList(topology.staleIds(), out, TopologySerializers.nodeId);
+
+ List<Shard> shards = topology.shards();
+
+ // need to loop twice; once to collect tables/ranges, and another to save shards
+ ImmutableUniqueList<TableId> tables;
Review Comment:
It is new to src, which to me is new to the PR. The expectations of src are different, and we will need to at least haggle about naming etc. But, even if it were already in src it is not a suitable data structure here as it imposes unnecessary costs in at least one but likely both directions.
For deserialisation, the object->int mapping is unused, so this is a costly waste of effort to construct.
For serialisation I don’t see why you need to sort the values? The normal approach here is to simply write them in the order they come, and ensure they are inserted into the hash map in that same order, so that the hash map contains their index.
That is, on deserialization you simply build the original array/list and lookup by index, and on serialisation you serialise the array/list and build a lookup for finding the index to encode. This pattern is used extensively throughout the code base.
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2194524481
##########
src/java/org/apache/cassandra/utils/SimpleBitSetSerializer.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * 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 accord.utils.SimpleBitSet;
+import org.apache.cassandra.db.TypeSizes;
+import org.apache.cassandra.io.UnversionedSerializer;
+import org.apache.cassandra.io.util.DataInputPlus;
+import org.apache.cassandra.io.util.DataOutputPlus;
+
+public class SimpleBitSetSerializer implements UnversionedSerializer<SimpleBitSet>
+{
+ public static final SimpleBitSetSerializer instance = new SimpleBitSetSerializer();
+
+ @Override
+ public void serialize(SimpleBitSet t, DataOutputPlus out) throws IOException
+ {
+ long[] raw = SimpleBitSet.SerializationSupport.getArray(t);
+ out.writeUnsignedVInt32(raw.length);
+ for (int i = 0; i < raw.length; i++)
+ out.writeVInt(raw[i]);
Review Comment:
writeUnsignedVInt? Better particularly for small bitsets
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2198656557
##########
src/java/org/apache/cassandra/utils/SimpleBitSetSerializer.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * 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 accord.utils.SimpleBitSet;
+import org.apache.cassandra.db.TypeSizes;
+import org.apache.cassandra.io.UnversionedSerializer;
+import org.apache.cassandra.io.util.DataInputPlus;
+import org.apache.cassandra.io.util.DataOutputPlus;
+
+public class SimpleBitSetSerializer implements UnversionedSerializer<SimpleBitSet>
+{
+ public static final SimpleBitSetSerializer instance = new SimpleBitSetSerializer();
+
+ @Override
+ public void serialize(SimpleBitSet t, DataOutputPlus out) throws IOException
+ {
+ long[] raw = SimpleBitSet.SerializationSupport.getArray(t);
+ out.writeUnsignedVInt32(raw.length);
+ for (int i = 0; i < raw.length; i++)
+ out.writeVInt(raw[i]);
Review Comment:
done
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2194554294
##########
src/java/org/apache/cassandra/service/accord/serializers/TopologySerializers.java:
##########
@@ -159,4 +168,141 @@ public long serializedSize(Topology topology)
return size;
}
};
+
+ public static final UnversionedSerializer<Topology> compactTopology = new UnversionedSerializer<>()
+ {
+ @Override
+ public void serialize(Topology topology, DataOutputPlus out) throws IOException
+ {
+ out.writeLong(topology.epoch());
Review Comment:
suggestion: unsigned vint?
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2192756702
##########
src/java/org/apache/cassandra/service/accord/serializers/TopologySerializers.java:
##########
@@ -159,4 +167,127 @@ public long serializedSize(Topology topology)
return size;
}
};
+
+ public static final UnversionedSerializer<Topology> compactTopology = new UnversionedSerializer<>()
+ {
+ @Override
+ public void serialize(Topology topology, DataOutputPlus out) throws IOException
+ {
+ out.writeLong(topology.epoch());
+ CollectionSerializers.serializeList(topology.staleIds(), out, TopologySerializers.nodeId);
+
+ List<Shard> shards = topology.shards();
+
+ // need to loop twice; once to collect tables/ranges, and another to save shards
+ ImmutableUniqueList<TableId> tables;
Review Comment:
In fact, you can even do this dynamically - which is another common technique in our serializers. When you go to serialise a value, you deserialize the index, and if the index is some sentinel (e.g. 0), then you deserialize a new value and insert it into the List. Otherwise you lookup the relevant index that has already been deserialised. On serialization, when you lookup the index, if it isn't there you write the sentinel and then the value.
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell closed pull request #4202: CASSANDRA-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost
URL: https://github.com/apache/cassandra/pull/4202
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2151002903
##########
src/java/org/apache/cassandra/schema/TableId.java:
##########
@@ -402,6 +404,27 @@ public long serializedSize(TableId t, int version)
}
};
+ public static final UnversionedSerializer<TableId> compactComparableSerializer = new UnversionedSerializer<TableId>()
Review Comment:
when working with lists having fields rather than a serializer causes duplicate code, so created this serializer to be reused with collection serializer
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2151088681
##########
src/java/org/apache/cassandra/utils/ImmutableUniqueList.java:
##########
@@ -19,24 +19,39 @@
package org.apache.cassandra.utils;
import java.util.AbstractList;
-import java.util.AbstractSet;
import java.util.ArrayList;
import java.util.Collection;
-import java.util.Iterator;
import java.util.List;
import java.util.RandomAccess;
import org.agrona.collections.Object2IntHashMap;
-public class ImmutableUniqueList<T> extends AbstractList<T> implements RandomAccess
Review Comment:
this was in `test/unit` as i used this for tests, but moved it over to `src` to leverage. I also took the opportunity to cleanup a few things
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2155085497
##########
test/harry/main/org/apache/cassandra/harry/model/ASTSingleTableModel.java:
##########
@@ -1539,9 +1539,10 @@ private List<PrimaryKey> findByPartitionEq(LookupContext ctx)
private Clustering<ByteBuffer> key(Map<Symbol, Expression> values, ImmutableUniqueList<Symbol> columns)
{
+ if (columns.isEmpty()) return Clustering.EMPTY;
// same as keys, but only one possible value can happen
List<Clustering<ByteBuffer>> keys = keys(Maps.transformValues(values, Collections::singletonList), columns);
- Preconditions.checkState(keys.size() == 1, "Expected 1 key, but found %d", keys.size());
+ Preconditions.checkState(keys.size() == 1, "Expected 1 key, but found %s", keys.size());
Review Comment:
guava doesn't use `String.format` and uses `%s` for all arguments
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2155060409
##########
src/java/org/apache/cassandra/service/accord/serializers/TopologySerializers.java:
##########
@@ -159,4 +167,127 @@ public long serializedSize(Topology topology)
return size;
}
};
+
+ public static final UnversionedSerializer<Topology> compactTopology = new UnversionedSerializer<>()
+ {
+ @Override
+ public void serialize(Topology topology, DataOutputPlus out) throws IOException
+ {
+ out.writeLong(topology.epoch());
+ CollectionSerializers.serializeList(topology.staleIds(), out, TopologySerializers.nodeId);
+
+ List<Shard> shards = topology.shards();
+
+ // need to loop twice; once to collect tables/ranges, and another to save shards
+ ImmutableUniqueList<TableId> tables;
+ ImmutableUniqueList<TokenRange> ranges;
+ {
+ ImmutableUniqueList.Builder<TableId> tablesBuilder = ImmutableUniqueList.builder();
+ ImmutableUniqueList.Builder<TokenRange> rangesBuilder = ImmutableUniqueList.builder();
+ for (Shard shard : shards)
+ {
+ TokenRange range = (TokenRange) shard.range;
+ tablesBuilder.add(range.table());
+ rangesBuilder.add(range.withTable(TableId.UNDEFINED));
+ }
+ tables = tablesBuilder.buildAndClear();
+ ranges = rangesBuilder.buildAndClear();
+ }
+
+ CollectionSerializers.serializeList(tables, out, TableId.compactComparableSerializer);
+ CollectionSerializers.serializeList(ranges, out, TokenRange.noTableSerializer);
+
+ out.writeUnsignedVInt32(shards.size());
+ for (Shard shard : shards)
+ {
+ TokenRange range = (TokenRange) shard.range;
+ int tableIdx = tables.indexOf(range.table());
+ int rangeIdx = ranges.indexOf(range.withTable(TableId.UNDEFINED));
+ out.writeUnsignedVInt32(tableIdx);
+ out.writeUnsignedVInt32(rangeIdx);
+
+ CollectionSerializers.serializeList(shard.nodes, out, TopologySerializers.nodeId);
+ BitSet notInFastPath = SortedArrays.toBitSet(shard.nodes, shard.notInFastPath);
+ BitSetSerializer.instance.serialize(notInFastPath, out);
+ BitSet joining = SortedArrays.toBitSet(shard.nodes, shard.joining);
+ BitSetSerializer.instance.serialize(joining, out);
Review Comment:
for murmur partitioner this actually is one of the main savings
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2192752190
##########
src/java/org/apache/cassandra/service/accord/serializers/TopologySerializers.java:
##########
@@ -159,4 +167,127 @@ public long serializedSize(Topology topology)
return size;
}
};
+
+ public static final UnversionedSerializer<Topology> compactTopology = new UnversionedSerializer<>()
+ {
+ @Override
+ public void serialize(Topology topology, DataOutputPlus out) throws IOException
+ {
+ out.writeLong(topology.epoch());
+ CollectionSerializers.serializeList(topology.staleIds(), out, TopologySerializers.nodeId);
+
+ List<Shard> shards = topology.shards();
+
+ // need to loop twice; once to collect tables/ranges, and another to save shards
+ ImmutableUniqueList<TableId> tables;
Review Comment:
Note, this logic works just as well for the case of duplicate items. You just skip anything already written, that must exist in the map. That is, you do
```
if (null == indexLookup.putIfAbsent(value, indexLookup.size()))
out.write(value);
```
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2191021211
##########
src/java/org/apache/cassandra/service/accord/serializers/TopologySerializers.java:
##########
@@ -159,4 +167,127 @@ public long serializedSize(Topology topology)
return size;
}
};
+
+ public static final UnversionedSerializer<Topology> compactTopology = new UnversionedSerializer<>()
+ {
+ @Override
+ public void serialize(Topology topology, DataOutputPlus out) throws IOException
+ {
+ out.writeLong(topology.epoch());
+ CollectionSerializers.serializeList(topology.staleIds(), out, TopologySerializers.nodeId);
+
+ List<Shard> shards = topology.shards();
+
+ // need to loop twice; once to collect tables/ranges, and another to save shards
+ ImmutableUniqueList<TableId> tables;
+ ImmutableUniqueList<TokenRange> ranges;
+ {
+ ImmutableUniqueList.Builder<TableId> tablesBuilder = ImmutableUniqueList.builder();
+ ImmutableUniqueList.Builder<TokenRange> rangesBuilder = ImmutableUniqueList.builder();
+ for (Shard shard : shards)
+ {
+ TokenRange range = (TokenRange) shard.range;
+ tablesBuilder.add(range.table());
+ rangesBuilder.add(range.withTable(TableId.UNDEFINED));
+ }
+ tables = tablesBuilder.buildAndClear();
+ ranges = rangesBuilder.buildAndClear();
+ }
+
+ CollectionSerializers.serializeList(tables, out, TableId.compactComparableSerializer);
+ CollectionSerializers.serializeList(ranges, out, TokenRange.noTableSerializer);
+
+ out.writeUnsignedVInt32(shards.size());
+ for (Shard shard : shards)
+ {
+ TokenRange range = (TokenRange) shard.range;
+ int tableIdx = tables.indexOf(range.table());
+ int rangeIdx = ranges.indexOf(range.withTable(TableId.UNDEFINED));
+ out.writeUnsignedVInt32(tableIdx);
+ out.writeUnsignedVInt32(rangeIdx);
+
+ CollectionSerializers.serializeList(shard.nodes, out, TopologySerializers.nodeId);
+ BitSet notInFastPath = SortedArrays.toBitSet(shard.nodes, shard.notInFastPath);
+ BitSetSerializer.instance.serialize(notInFastPath, out);
+ BitSet joining = SortedArrays.toBitSet(shard.nodes, shard.joining);
+ BitSetSerializer.instance.serialize(joining, out);
Review Comment:
sorry, i wasn't clear. This specific line isn't what i am talking about, but the node ids are. joining is 1 byte in the common case now which should be in-line with previous version. But when something is joining its still 1 byte.
Same happens in the `notInFastPath`. This logic is the most repeated so got the largest savings
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2151091874
##########
src/java/org/apache/cassandra/utils/BitSetSerializer.java:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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 java.util.BitSet;
+
+import org.apache.cassandra.db.TypeSizes;
+import org.apache.cassandra.io.UnversionedSerializer;
+import org.apache.cassandra.io.util.DataInputPlus;
+import org.apache.cassandra.io.util.DataOutputPlus;
+
+public class BitSetSerializer implements UnversionedSerializer<BitSet>
+{
+ public static final BitSetSerializer instance = new BitSetSerializer();
+
+ @Override
+ public void serialize(BitSet t, DataOutputPlus out) throws IOException
+ {
+ byte[] bytes = t.toByteArray();
Review Comment:
in the topology tests i never saw more than 2 bytes; 1 for the length, and 1 for the data.; the common case is 1 byte (empty)
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#issuecomment-2982073618
`CompactTopology` is now gone, and the `compactTopology` serializer in-lines all the work.
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2185150523
##########
src/java/org/apache/cassandra/service/accord/serializers/TopologySerializers.java:
##########
@@ -159,4 +167,127 @@ public long serializedSize(Topology topology)
return size;
}
};
+
+ public static final UnversionedSerializer<Topology> compactTopology = new UnversionedSerializer<>()
+ {
+ @Override
+ public void serialize(Topology topology, DataOutputPlus out) throws IOException
+ {
+ out.writeLong(topology.epoch());
+ CollectionSerializers.serializeList(topology.staleIds(), out, TopologySerializers.nodeId);
+
+ List<Shard> shards = topology.shards();
+
+ // need to loop twice; once to collect tables/ranges, and another to save shards
+ ImmutableUniqueList<TableId> tables;
+ ImmutableUniqueList<TokenRange> ranges;
+ {
+ ImmutableUniqueList.Builder<TableId> tablesBuilder = ImmutableUniqueList.builder();
+ ImmutableUniqueList.Builder<TokenRange> rangesBuilder = ImmutableUniqueList.builder();
+ for (Shard shard : shards)
+ {
+ TokenRange range = (TokenRange) shard.range;
+ tablesBuilder.add(range.table());
+ rangesBuilder.add(range.withTable(TableId.UNDEFINED));
+ }
+ tables = tablesBuilder.buildAndClear();
+ ranges = rangesBuilder.buildAndClear();
+ }
+
+ CollectionSerializers.serializeList(tables, out, TableId.compactComparableSerializer);
+ CollectionSerializers.serializeList(ranges, out, TokenRange.noTableSerializer);
+
+ out.writeUnsignedVInt32(shards.size());
+ for (Shard shard : shards)
+ {
+ TokenRange range = (TokenRange) shard.range;
+ int tableIdx = tables.indexOf(range.table());
+ int rangeIdx = ranges.indexOf(range.withTable(TableId.UNDEFINED));
+ out.writeUnsignedVInt32(tableIdx);
+ out.writeUnsignedVInt32(rangeIdx);
+
+ CollectionSerializers.serializeList(shard.nodes, out, TopologySerializers.nodeId);
+ BitSet notInFastPath = SortedArrays.toBitSet(shard.nodes, shard.notInFastPath);
+ BitSetSerializer.instance.serialize(notInFastPath, out);
+ BitSet joining = SortedArrays.toBitSet(shard.nodes, shard.joining);
+ BitSetSerializer.instance.serialize(joining, out);
Review Comment:
that's surprising - should't this collection normally be empty?
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2155093318
##########
test/unit/org/apache/cassandra/service/accord/serializers/TopologySerializersTest.java:
##########
@@ -57,4 +72,98 @@ public void topology()
Serializers.testSerde(output, TopologySerializers.topology, expected.forNode(node));
});
}
+
+ /**
+ * This test focuses on correctness, given any random topology does the serde operation work as expected?
+ */
+ @Test
+ public void compactTopology()
+ {
+ @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.compactTopology, expected);
+
+ for (Node.Id node : expected.nodes())
+ Serializers.testSerde(output, TopologySerializers.compactTopology, expected.forNode(node));
+ });
+ }
+
+ /**
+ * This test tries to create Topologies that would look closer to what one would expect in a production environment; namily each table having the same ranges.
+ */
+ @Test
+ public void compactTopologyAreCompact()
+ {
+ Gen<Ranges> rangeGen = AccordGenerators.ranges(Gens.lists(Generators.toGen(CassandraGenerators.TABLE_ID_GEN)).ofSizeBetween(2, 10),
+ AccordGenerators.partitioner(),
+ Gens.ints().between(2, 1000));
+ Gen<Topology> gen = AccordGenerators.topologyGen(AccordGens.epochs(), rangeGen);
+ @SuppressWarnings({ "resource", "IOResourceOpenedButNotSafelyClosed" }) DataOutputBuffer output = new DataOutputBuffer();
+ CompactCollector collector = new CompactCollector();
Review Comment:
this is what i used in the JIRA/Body to show the savings. Was helpful to compare changes. Its mostly dead code in CI though
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2155100141
##########
test/unit/org/apache/cassandra/utils/AccordGenerators.java:
##########
@@ -757,6 +780,67 @@ public static Gen<Topology> topologyGen(Gen.LongGen epochGen, Gen<Ranges> ranges
};
}
+ public static Gen<FetchTopologies> fetchTopologiesGen()
+ {
+ Gen.LongGen epochGen = AccordGens.epochs();
+ Gen.LongGen maxEpochGen = rs -> {
+ if (rs.decide(0.3))
+ return Long.MAX_VALUE;
+ return epochGen.nextLong(rs);
+ };
+ return rs -> {
+ long a = epochGen.nextLong(rs);
+ long b = maxEpochGen.nextLong(rs);
+ while (a == b)
+ b = maxEpochGen.nextLong(rs);
+ if (a > b)
+ {
+ long tmp = a;
+ a = b;
+ b = tmp;
+ }
+ return new FetchTopologies(a, b);
+ };
+ }
+
+ public static Gen<TopologyManager.TopologyRange> topologyRangeGen()
+ {
+ Gen.LongGen epochGen = AccordGens.epochs();
+ return rs -> {
+ // settle on 1 partitioner
+ IPartitioner partitioner = partitioner().next(rs);
+ Supplier<Topology> topologyGen = () -> {
+ if (rs.decide(.3)) return Topology.EMPTY;
Review Comment:
`topologyGen` doesn't support empty topologies, but that actually matters for `TopologyRange` so made sure to include
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2155065069
##########
src/java/org/apache/cassandra/utils/ImmutableUniqueList.java:
##########
@@ -107,6 +119,23 @@ public int size()
return values.length;
}
+ @Override
+ public boolean equals(Object o)
Review Comment:
AST model depends on checking existence with a set, so support defining equals with set based just on the `containsAll` property rather than the order property.
--
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-20715: Accord: Topology serializer has a lot of repeated data, can dedup to shrink the cost [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4202:
URL: https://github.com/apache/cassandra/pull/4202#discussion_r2191106842
##########
src/java/org/apache/cassandra/service/accord/serializers/TopologySerializers.java:
##########
@@ -159,4 +167,127 @@ public long serializedSize(Topology topology)
return size;
}
};
+
+ public static final UnversionedSerializer<Topology> compactTopology = new UnversionedSerializer<>()
+ {
+ @Override
+ public void serialize(Topology topology, DataOutputPlus out) throws IOException
+ {
+ out.writeLong(topology.epoch());
+ CollectionSerializers.serializeList(topology.staleIds(), out, TopologySerializers.nodeId);
+
+ List<Shard> shards = topology.shards();
+
+ // need to loop twice; once to collect tables/ranges, and another to save shards
+ ImmutableUniqueList<TableId> tables;
Review Comment:
ok i took a look again and its the following
1) `ImmutableUniqueList` is only *new* in the PR as git didn't see this as a migration; moved from `test` to `src`
2) serializing the list; I need to write the data in insertion order. `Object2IntHashMap` isn't based off insertion order and isn't stable, so the serialization needs to do one of the following: copy the keys to a list and sorted by value, or maintain a `index -> type` mapping. In both cases I argue `ImmutableUniqueList` is more efficient (the extra list doesn't need to be sorted, and is cheaper than the `index -> type` mapping).
--
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