ibessonov commented on code in PR #5468:
URL: https://github.com/apache/ignite-3/pull/5468#discussion_r2011898394
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogTableDescriptorSerializers.java:
##########
@@ -191,7 +195,8 @@ static class TableDescriptorSerializerV2 implements
CatalogObjectSerializer<Cata
public CatalogTableDescriptor readFrom(CatalogObjectDataInput input)
throws IOException {
int id = input.readVarIntAsInt();
String name = input.readUTF();
- long updateToken = input.readVarInt();
+ long updateTimestampLong = input.readVarInt();
+ HybridTimestamp updateTimestamp = updateTimestampLong == 0 ?
MIN_VALUE : hybridTimestamp(updateTimestampLong);
Review Comment:
Why is this necessary? How can it happen?
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/rebalance/RebalanceMinimumRequiredTimeProviderImpl.java:
##########
@@ -172,9 +156,9 @@ Map<Integer, NavigableMap<Long, CatalogZoneDescriptor>>
allZonesByTimestamp(
NavigableMap<Long, CatalogZoneDescriptor> map =
allZones.computeIfAbsent(zone.id(), id -> new TreeMap<>());
if (map.isEmpty() ||
updateRequiresAssignmentsRecalculation(map.lastEntry().getValue(), zone)) {
- map.put(catalog.time(), zone);
+ map.put(zone.updateTimestamp().longValue(), zone);
Review Comment:
Can we use `HybridTimestamp` here for keys? This will make code more
readable. This class is `Comparable`, so no worries about that. Please update
all the usages in this class, thank you!
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/storage/TestCatalogObjectDescriptors.java:
##########
@@ -128,10 +130,11 @@ static List<CatalogTableDescriptor>
tables(TestDescriptorState state) {
list.add(list.get(0).newDescriptor(
table1.name() + "_1", 2,
columns(state).subList(0, 10),
- 1232L,
+ hybridTimestamp(1232L),
"S1")
);
- list.add(list.get(list.size() - 1).newDescriptor(table1.name() + "_2",
3, columns(state).subList(0, 20), 21232L, "S1"));
+ list.add(list.get(list.size() - 1).newDescriptor(table1.name() + "_2",
3, columns(state).subList(0, 20), hybridTimestamp(21232L),
+ "S1"));
Review Comment:
Very unconventional formatting, please make it look better
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/descriptors/CatalogZoneDescriptor.java:
##########
@@ -57,7 +58,7 @@ public class CatalogZoneDescriptor extends
CatalogObjectDescriptor implements Ma
* Returns {@code true} if zone upgrade will lead to assignments
recalculation.
*/
public static boolean
updateRequiresAssignmentsRecalculation(CatalogZoneDescriptor oldDescriptor,
CatalogZoneDescriptor newDescriptor) {
- if (oldDescriptor.updateToken() == newDescriptor.updateToken()) {
+ if (oldDescriptor.updateTimestamp() ==
newDescriptor.updateTimestamp()) {
Review Comment:
Why is it OK to use reference equality here? This might be a bug, **every**
reference equality check must be justified with a 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
