This is an automated email from the ASF dual-hosted git repository. alexey pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/kudu.git
commit 4814bb2be2809e56bb9eda58c727ce8970ac9f0f Author: Alexey Serbin <[email protected]> AuthorDate: Fri Oct 13 15:35:12 2023 -0700 [tablet] a small clean up on 'disable_compaction' While changing the code in the Tablet class, I found the naming of the methods related to the 'disable_compaction' property (a) confusing and (b) non-compliant with the project's style guide. This patch addresses the issue. I also extended the test coverage for the related functionality a little bit. Change-Id: I209a58505ea655a94563c51ba00b86ee24ea3e40 Reviewed-on: http://gerrit.cloudera.org:8080/20577 Tested-by: Alexey Serbin <[email protected]> Reviewed-by: KeDeng <[email protected]> Reviewed-by: Yifan Zhang <[email protected]> --- src/kudu/integration-tests/alter_table-test.cc | 57 ++++++++++++++++---------- src/kudu/tablet/tablet.cc | 6 +-- src/kudu/tablet/tablet.h | 6 +-- src/kudu/tablet/tablet_mm_ops-test.cc | 12 +++--- src/kudu/tablet/tablet_mm_ops.cc | 10 ++--- src/kudu/tablet/tablet_mm_ops.h | 7 ++-- 6 files changed, 56 insertions(+), 42 deletions(-) diff --git a/src/kudu/integration-tests/alter_table-test.cc b/src/kudu/integration-tests/alter_table-test.cc index ded8bff3b..cf75a5fdc 100644 --- a/src/kudu/integration-tests/alter_table-test.cc +++ b/src/kudu/integration-tests/alter_table-test.cc @@ -20,6 +20,7 @@ #include <cstddef> #include <cstdint> #include <functional> +#include <initializer_list> #include <map> #include <memory> #include <optional> @@ -379,10 +380,6 @@ class AlterTableTest : public KuduTest { } } - void CheckDisableCompaction(const bool& disable_compaction) { - ASSERT_EQ(tablet_replica_->tablet()->disable_compaction(), disable_compaction); - } - protected: virtual int num_replicas() const { return 1; } virtual int num_tservers() const { return 1; } @@ -2526,30 +2523,46 @@ TEST_F(AlterTableTest, TestMaintenancePriorityAlter) { NO_FATALS(CheckMaintenancePriority(0)); } -TEST_F(AlterTableTest, TestDisableCompactAlter) { - // Default disable_compaction. - NO_FATALS(CheckDisableCompaction(false)); +TEST_F(AlterTableTest, DisableCompactionAlter) { + const auto* tablet = tablet_replica_->tablet(); + ASSERT_NE(nullptr, tablet); + + // Check the default setting for the 'disable_compaction' property. + ASSERT_TRUE(tablet->compaction_enabled()); - // Set disable_compaction true. unique_ptr<KuduTableAlterer> table_alterer(client_->NewTableAlterer(kTableName)); - ASSERT_OK(table_alterer->AlterExtraConfig( - {{"kudu.table.disable_compaction", "true"}})->Alter()); - NO_FATALS(CheckDisableCompaction(true)); - // Reset to default disable_compaction. - ASSERT_OK(table_alterer->AlterExtraConfig( - {{"kudu.table.disable_compaction", ""}})->Alter()); - NO_FATALS(CheckDisableCompaction(false)); + // Set the property to 'true'. + for (const auto& val : {"true", "TRUE", "True", "TrUe", "TRUe"}) { + SCOPED_TRACE(Substitute("setting value to: $0", val)); + ASSERT_OK(table_alterer->AlterExtraConfig( + {{"kudu.table.disable_compaction", val}})->Alter()); + ASSERT_FALSE(tablet->compaction_enabled()); + } - // Set disable_compaction false. + // Reset the property to the default setting by supplying an empty string + // for the property's value. ASSERT_OK(table_alterer->AlterExtraConfig( - {{"kudu.table.disable_compaction", "false"}})->Alter()); - NO_FATALS(CheckDisableCompaction(false)); + {{"kudu.table.disable_compaction", ""}})->Alter()); + ASSERT_TRUE(tablet->compaction_enabled()); + + // Explicitly set the 'disable_compaction' property to 'false'. + for (const auto& val : {"false", "FALSE", "False", "FaLsE", "fALSE"}) { + SCOPED_TRACE(Substitute("setting value to: $0", val)); + ASSERT_OK(table_alterer->AlterExtraConfig( + {{"kudu.table.disable_compaction", "false"}})->Alter()); + ASSERT_TRUE(tablet->compaction_enabled()); + } - // Set to invalid string. - Status s = table_alterer->AlterExtraConfig( - {{"kudu.table.disable_compaction", "ok"}})->Alter(); - ASSERT_TRUE(s.IsInvalidArgument()); + // Try setting the property to an invalid value. + for (const auto& val : {"ok", "ttrue", "trueue", "falsee", "fallse"}) { + SCOPED_TRACE(Substitute("trying to set value: $0", val)); + const auto s = table_alterer->AlterExtraConfig( + {{"kudu.table.disable_compaction", val}})->Alter(); + ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString(); + // The prior setting should be still effective. + ASSERT_TRUE(tablet->compaction_enabled()); + } } TEST_F(AlterTableTest, AddAndRemoveImmutableAttribute) { diff --git a/src/kudu/tablet/tablet.cc b/src/kudu/tablet/tablet.cc index 666f5a4f1..d7a1e88b5 100644 --- a/src/kudu/tablet/tablet.cc +++ b/src/kudu/tablet/tablet.cc @@ -1891,12 +1891,12 @@ Status Tablet::PickRowSetsToCompact(RowSetsInCompaction *picked, return Status::OK(); } -bool Tablet::disable_compaction() const { +bool Tablet::compaction_enabled() const { const auto& extra_config = metadata_->extra_config(); if (extra_config && extra_config->has_disable_compaction()) { - return extra_config->disable_compaction(); + return !extra_config->disable_compaction(); } - return false; + return true; } void Tablet::GetRowSetsForTests(RowSetVector* out) { diff --git a/src/kudu/tablet/tablet.h b/src/kudu/tablet/tablet.h index 63fe284d0..8d48967ae 100644 --- a/src/kudu/tablet/tablet.h +++ b/src/kudu/tablet/tablet.h @@ -510,9 +510,9 @@ class Tablet { std::string LogPrefix() const; - // Return false if the tablets need to compact, - // otherwise return true. - bool disable_compaction() const; + // Return 'true' if the tablet's data may be compacted, + // otherwise return 'false'. + bool compaction_enabled() const; // Return the default bloom filter sizing parameters, configured by server flags. static BloomFilterSizing DefaultBloomSizing(); diff --git a/src/kudu/tablet/tablet_mm_ops-test.cc b/src/kudu/tablet/tablet_mm_ops-test.cc index b7bb778f7..d391a25b2 100644 --- a/src/kudu/tablet/tablet_mm_ops-test.cc +++ b/src/kudu/tablet/tablet_mm_ops-test.cc @@ -142,7 +142,7 @@ class KuduTabletMmOpsTest : public TabletTestBase<IntKeyTestSetup<INT64>> { TEST_F(KuduTabletMmOpsTest, TestCompactRowSetsOpCacheStats) { CompactRowSetsOp op(tablet().get()); - ASSERT_FALSE(op.DisableCompaction()); + ASSERT_TRUE(op.compaction_enabled()); NO_FATALS(TestFirstCall(&op)); auto* m = tablet()->metrics(); NO_FATALS(TestAffectedMetrics(&op, { m->flush_mrs_duration, @@ -155,13 +155,13 @@ TEST_F(KuduTabletMmOpsTest, TestDisableCompactRowSetsOp) { TableExtraConfigPB extra_config; extra_config.set_disable_compaction(true); NO_FATALS(AlterSchema(*harness_->tablet()->schema(), std::make_optional(extra_config))); - ASSERT_TRUE(op.DisableCompaction()); + ASSERT_FALSE(op.compaction_enabled()); NO_FATALS(TestNoAffectedMetrics(&op)); } TEST_F(KuduTabletMmOpsTest, TestMinorDeltaCompactionOpCacheStats) { MinorDeltaCompactionOp op(tablet().get()); - ASSERT_FALSE(op.DisableCompaction()); + ASSERT_TRUE(op.compaction_enabled()); NO_FATALS(TestFirstCall(&op)); NO_FATALS(TestAffectedMetrics(&op, { tablet()->metrics()->flush_mrs_duration, tablet()->metrics()->flush_dms_duration, @@ -174,13 +174,13 @@ TEST_F(KuduTabletMmOpsTest, TestDisableMinorDeltaCompactionOp) { TableExtraConfigPB extra_config; extra_config.set_disable_compaction(true); NO_FATALS(AlterSchema(*harness_->tablet()->schema(), std::make_optional(extra_config))); - ASSERT_TRUE(op.DisableCompaction()); + ASSERT_FALSE(op.compaction_enabled()); NO_FATALS(TestNoAffectedMetrics(&op)); } TEST_F(KuduTabletMmOpsTest, TestMajorDeltaCompactionOpCacheStats) { MajorDeltaCompactionOp op(tablet().get()); - ASSERT_FALSE(op.DisableCompaction()); + ASSERT_TRUE(op.compaction_enabled()); NO_FATALS(TestFirstCall(&op)); NO_FATALS(TestAffectedMetrics(&op, { tablet()->metrics()->flush_mrs_duration, tablet()->metrics()->flush_dms_duration, @@ -194,7 +194,7 @@ TEST_F(KuduTabletMmOpsTest, TestDisableMajorDeltaCompactionOp) { TableExtraConfigPB extra_config; extra_config.set_disable_compaction(true); NO_FATALS(AlterSchema(*harness_->tablet()->schema(), std::make_optional(extra_config))); - ASSERT_TRUE(op.DisableCompaction()); + ASSERT_FALSE(op.compaction_enabled()); NO_FATALS(TestNoAffectedMetrics(&op)); } } // namespace tablet diff --git a/src/kudu/tablet/tablet_mm_ops.cc b/src/kudu/tablet/tablet_mm_ops.cc index c931cb831..ddf04bc46 100644 --- a/src/kudu/tablet/tablet_mm_ops.cc +++ b/src/kudu/tablet/tablet_mm_ops.cc @@ -119,8 +119,8 @@ int32_t TabletOpBase::priority() const { return priority; } -bool TabletOpBase::DisableCompaction() const { - return tablet_->disable_compaction(); +bool TabletOpBase::compaction_enabled() const { + return tablet_->compaction_enabled(); } //////////////////////////////////////////////////////////// @@ -136,7 +136,7 @@ CompactRowSetsOp::CompactRowSetsOp(Tablet* tablet) } void CompactRowSetsOp::UpdateStats(MaintenanceOpStats* stats) { - if (PREDICT_FALSE(!FLAGS_enable_rowset_compaction || DisableCompaction())) { + if (PREDICT_FALSE(!FLAGS_enable_rowset_compaction || !compaction_enabled())) { KLOG_EVERY_N_SECS(WARNING, FLAGS_update_stats_log_throttling_interval_sec) << Substitute("Rowset compaction is disabled (check --enable_rowset_compaction " "and disable_compaction in extra_config for tablet:$0)", tablet_->tablet_id()); @@ -217,7 +217,7 @@ MinorDeltaCompactionOp::MinorDeltaCompactionOp(Tablet* tablet) } void MinorDeltaCompactionOp::UpdateStats(MaintenanceOpStats* stats) { - if (PREDICT_FALSE(!FLAGS_enable_minor_delta_compaction || DisableCompaction())) { + if (PREDICT_FALSE(!FLAGS_enable_minor_delta_compaction || !compaction_enabled())) { KLOG_EVERY_N_SECS(WARNING, FLAGS_update_stats_log_throttling_interval_sec) << Substitute("Minor delta compaction is disabled (check --enable_minor_delta_compaction " "and disable_compaction in extra_config for tablet:$0)", tablet_->tablet_id()); @@ -301,7 +301,7 @@ MajorDeltaCompactionOp::MajorDeltaCompactionOp(Tablet* tablet) } void MajorDeltaCompactionOp::UpdateStats(MaintenanceOpStats* stats) { - if (PREDICT_FALSE(!FLAGS_enable_major_delta_compaction || DisableCompaction())) { + if (PREDICT_FALSE(!FLAGS_enable_major_delta_compaction || !compaction_enabled())) { KLOG_EVERY_N_SECS(WARNING, FLAGS_update_stats_log_throttling_interval_sec) << Substitute("Major delta compaction is disabled (check --enable_major_delta_compaction " "and disable_compaction in extra_config for tablet:$0)", tablet_->tablet_id()); diff --git a/src/kudu/tablet/tablet_mm_ops.h b/src/kudu/tablet/tablet_mm_ops.h index 3c9829d2f..3f35899eb 100644 --- a/src/kudu/tablet/tablet_mm_ops.h +++ b/src/kudu/tablet/tablet_mm_ops.h @@ -40,9 +40,10 @@ class TabletOpBase : public MaintenanceOp { TabletOpBase(std::string name, IOUsage io_usage, Tablet* tablet); std::string LogPrefix() const; - // Return false if the tablets need to compact, - // otherwise return true. - bool DisableCompaction() const; + // Return 'true' if the tablet's data may be compacted (i.e. the tablet + // should be considered as a candidate for compaction), + // otherwise return 'false'. + bool compaction_enabled() const; protected: int32_t priority() const override;
