From 7a1fbb38f9d9dc0c642e1ddbbecb41ca61e9fa8c Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Thu, 30 Nov 2023 17:25:13 +0800 Subject: [PATCH] sstable: order uuid-based generation as timeuuid under most circumstances, we don't care the ordering of the sstable identifiers, as they are just identifiers. so, as long as they can be compared, we are good. but we have tests with expect that the sstables can be ordered by the time they are created. for instance, sstable_run_based_compaction_test has this expectaion. before this change, we compare two UUID-based generations by its (MSB, LSB) lexicographically. but UUID v1 put the lower bits of the timestamp at the higher bits of MSB, so the ordering of the "time" in timeuuid is not preserved when comparing the UUID-based generations. this breaks the test of sstable_run_based_compaction_test, which feeds the sstables to be compacted in a set, and the set is ordered with the generation of the sstables. after this change, we consider the UUID-based generation as a timeuuid when comparing them. Fixes #16215 Signed-off-by: Kefu Chai Closes scylladb/scylladb#16238 --- sstables/generation_type.hh | 5 +++-- test/boost/sstable_generation_test.cc | 16 ++++++++++++++-- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/sstables/generation_type.hh b/sstables/generation_type.hh index 073a36296b..f29f96cadb 100644 --- a/sstables/generation_type.hh +++ b/sstables/generation_type.hh @@ -112,9 +112,10 @@ public: return _value.timestamp() != 0; } std::strong_ordering operator<=>(const generation_type& other) const noexcept { - if (bool(*this) && is_uuid_based() && + // preserve the ordering as a timeuuid + if (bool(*this) && this->is_uuid_based() && bool(other) && other.is_uuid_based()) { - return this->_value <=> other._value; + return timeuuid_tri_compare(this->_value, other._value); } int_t lhs = 0, rhs = 0; if (bool(*this) && !is_uuid_based()) { diff --git a/test/boost/sstable_generation_test.cc b/test/boost/sstable_generation_test.cc index c40186f61c..e7675cd2c2 100644 --- a/test/boost/sstable_generation_test.cc +++ b/test/boost/sstable_generation_test.cc @@ -89,12 +89,24 @@ BOOST_AUTO_TEST_CASE(compare) { BOOST_CHECK_GT(sstables::generation_type(42), sstables::generation_type(41)); BOOST_CHECK_LT(sstables::generation_type(41), sstables::generation_type(42)); BOOST_CHECK_EQUAL(sstables::generation_type(42), sstables::generation_type(42)); - // the ordering of uuid based generation does not matter, but we should be - // able to use them as key in an associative container + // if two identifiers are compared, we consider them as timeuuid, which means: + // + // 1. they can be used as key in an associative container + // 2. the newer timeuuids are order after the older ones + + // two UUIDs with the same timestamps but different clock seq and node BOOST_CHECK_NE(sstables::generation_type::from_string("3fw2_0tj4_46w3k2cpidnirvjy7k"), sstables::generation_type::from_string("3fw2_0tj4_46w3k2cpidnirvjy7z")); + // the return value from_string() of the same string representation should + // be equal BOOST_CHECK_EQUAL(sstables::generation_type::from_string(uuid), sstables::generation_type::from_string(uuid)); + // two UUIDs with different timestamps but the same clock seq and node, + // their timestamps are: + // - 2023-11-24 23:41:56 + // - 2023-11-24 23:41:57 + BOOST_CHECK_LE(sstables::generation_type::from_string("3gbc_17lw_3tlpc2fe8ko69yav6u"), + sstables::generation_type::from_string("3gbc_17lx_59opc2fe8ko69yav6u")); // all invalid identifiers should be equal BOOST_CHECK_EQUAL(sstables::generation_type{}, sstables::generation_type{}); BOOST_CHECK_NE(sstables::generation_type{},