Skip to content

Commit c780b50

Browse files
paolococchipaolococchi
committed
MB-48529: Remove unnecessary/test-only CheckpointConfig ctor
Change-Id: If547b06c8ef13de8f0eab4d6c865e43f71f1269d Reviewed-on: http://review.couchbase.org/c/kv_engine/+/163786 Tested-by: Paolo Cocchi <[email protected]> Reviewed-by: James H <[email protected]>
1 parent 3392c6e commit c780b50

File tree

5 files changed

+50
-123
lines changed

5 files changed

+50
-123
lines changed

‎engines/ep/src/checkpoint_config.cc‎

Lines changed: 7 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -67,27 +67,14 @@ class CheckpointConfig::ChangeListener : public ValueChangedListener {
6767
CheckpointConfig& config;
6868
};
6969

70-
CheckpointConfig::CheckpointConfig(rel_time_t period,
71-
size_t max_items,
72-
size_t max_ckpts,
73-
bool item_based_new_ckpt,
74-
bool persistence_enabled,
75-
CheckpointRemoval checkpoint_removal_mode)
76-
: checkpointPeriod(period),
77-
checkpointMaxItems(max_items),
78-
maxCheckpoints(max_ckpts),
79-
itemNumBasedNewCheckpoint(item_based_new_ckpt),
80-
persistenceEnabled(persistence_enabled),
81-
checkpointRemovalMode(checkpoint_removal_mode) {
82-
}
83-
8470
CheckpointConfig::CheckpointConfig(Configuration& config)
85-
: CheckpointConfig(config.getChkPeriod(),
86-
config.getChkMaxItems(),
87-
config.getMaxCheckpoints(),
88-
config.isItemNumBasedNewChk(),
89-
config.getBucketType() == "persistent",
90-
mode_from_string(config.getCheckpointRemovalMode())) {
71+
: checkpointPeriod(config.getChkPeriod()),
72+
checkpointMaxItems(config.getChkMaxItems()),
73+
maxCheckpoints(config.getMaxCheckpoints()),
74+
itemNumBasedNewCheckpoint(config.isItemNumBasedNewChk()),
75+
persistenceEnabled(config.getBucketType() == "persistent"),
76+
checkpointRemovalMode(
77+
mode_from_string(config.getCheckpointRemovalMode())) {
9178
}
9279

9380
void CheckpointConfig::addConfigChangeListener(

‎engines/ep/src/checkpoint_config.h‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,12 +27,6 @@ class CheckpointConfig {
2727
public:
2828
// Test only
2929
CheckpointConfig() = default;
30-
CheckpointConfig(rel_time_t period,
31-
size_t max_items,
32-
size_t max_ckpts,
33-
bool item_based_new_ckpt,
34-
bool persistence_enabled,
35-
CheckpointRemoval checkpoint_removal_mode);
3630

3731
explicit CheckpointConfig(Configuration& config);
3832

‎engines/ep/tests/module_tests/checkpoint_test.cc‎

Lines changed: 37 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ void CheckpointTest::SetUp() {
5252
// EagerCheckpointDisposalTest covers eager tests.
5353
config.parseConfiguration("checkpoint_removal_mode=lazy",
5454
get_mock_server_api());
55-
recreateCheckpointConfig();
55+
checkpoint_config = CheckpointConfig(config);
5656
VBucketTest::SetUp();
5757
createManager();
5858
}
@@ -61,20 +61,6 @@ void CheckpointTest::TearDown() {
6161
VBucketTest::TearDown();
6262
}
6363

64-
void CheckpointTest::recreateCheckpointConfig() {
65-
checkpoint_config = CheckpointConfig(
66-
DEFAULT_CHECKPOINT_PERIOD,
67-
DEFAULT_CHECKPOINT_ITEMS,
68-
config.getMaxCheckpoints(),
69-
true /*itemNumBasedNewCheckpoint*/,
70-
(getVbType() == VBType::Persistent ? true
71-
: false) /*persistenceEnabled*/,
72-
config.getCheckpointRemovalMode() ==
73-
::to_string(CheckpointRemoval::Eager)
74-
? CheckpointRemoval::Eager
75-
: CheckpointRemoval::Lazy);
76-
}
77-
7864
void CheckpointTest::createManager(int64_t lastSeqno) {
7965
ASSERT_TRUE(vbucket);
8066
range = {0, 0};
@@ -412,13 +398,8 @@ TEST_P(CheckpointTest, getItems_MemoryDiskSnapshots) {
412398
TEST_P(CheckpointTest, ItemBasedCheckpointCreation) {
413399
// Size down the default number of items to create a new checkpoint and
414400
// recreate the manager
415-
this->checkpoint_config =
416-
CheckpointConfig(DEFAULT_CHECKPOINT_PERIOD,
417-
MIN_CHECKPOINT_ITEMS,
418-
/*numCheckpoints*/ 2,
419-
/*itemBased*/ true,
420-
persistent() /*persistenceEnabled*/,
421-
CheckpointRemoval::Lazy);
401+
config.setChkMaxItems(MIN_CHECKPOINT_ITEMS);
402+
checkpoint_config = CheckpointConfig(config);
422403
createManager();
423404

424405
// Create one less than the number required to create a new checkpoint.
@@ -570,15 +551,12 @@ TEST_P(CheckpointTest, CursorOffsetOnCheckpointClose) {
570551

571552
// Test the getNextItemsForCursor()
572553
TEST_P(CheckpointTest, ItemsForCheckpointCursor) {
573-
// We want to have items across 2 checkpoints. Size down the default number
574-
// of items to create a new checkpoint and recreate the manager.
575-
this->checkpoint_config =
576-
CheckpointConfig(DEFAULT_CHECKPOINT_PERIOD,
577-
MIN_CHECKPOINT_ITEMS,
578-
/*numCheckpoints*/ 2,
579-
/*itemBased*/ true,
580-
persistent() /*persistenceEnabled*/,
581-
CheckpointRemoval::Lazy);
554+
// Size down the default number of items to create a new checkpoint and
555+
// recreate the manager.
556+
config.setChkMaxItems(MIN_CHECKPOINT_ITEMS);
557+
checkpoint_config = CheckpointConfig(config);
558+
// Also, we want to have items across 2 checkpoints.
559+
ASSERT_EQ(2, checkpoint_config.getMaxCheckpoints());
582560
createManager();
583561

584562
/* Add items such that we have 2 checkpoints */
@@ -629,15 +607,12 @@ TEST_P(CheckpointTest, ItemsForCheckpointCursor) {
629607
// Test getNextItemsForCursor() when it is limited to fewer items than exist
630608
// in total. Cursor should only advanced to the start of the 2nd checkpoint.
631609
TEST_P(CheckpointTest, ItemsForCheckpointCursorLimited) {
632-
/* We want to have items across 2 checkpoints. Size down the default number
633-
of items to create a new checkpoint and recreate the manager */
634-
this->checkpoint_config =
635-
CheckpointConfig(DEFAULT_CHECKPOINT_PERIOD,
636-
MIN_CHECKPOINT_ITEMS,
637-
/*numCheckpoints*/ 2,
638-
/*itemBased*/ true,
639-
persistent() /*persistenceEnabled*/,
640-
CheckpointRemoval::Lazy);
610+
// Size down the default number of items to create a new checkpoint and
611+
// recreate the manager.
612+
config.setChkMaxItems(MIN_CHECKPOINT_ITEMS);
613+
checkpoint_config = CheckpointConfig(config);
614+
// Also, we want to have items across 2 checkpoints.
615+
ASSERT_EQ(2, checkpoint_config.getMaxCheckpoints());
641616
createManager();
642617

643618
/* Add items such that we have 2 checkpoints */
@@ -672,15 +647,12 @@ TEST_P(CheckpointTest, DiskCheckpointStrictItemLimit) {
672647
if (!persistent()) {
673648
return;
674649
}
675-
/* We want to have items across 2 checkpoints. Size down the default number
676-
of items to create a new checkpoint and recreate the manager */
677-
this->checkpoint_config =
678-
CheckpointConfig(DEFAULT_CHECKPOINT_PERIOD,
679-
MIN_CHECKPOINT_ITEMS,
680-
/*numCheckpoints*/ 2,
681-
/*itemBased*/ true,
682-
persistent() /*persistenceEnabled*/,
683-
CheckpointRemoval::Lazy);
650+
// Size down the default number of items to create a new checkpoint and
651+
// recreate the manager.
652+
config.setChkMaxItems(MIN_CHECKPOINT_ITEMS);
653+
checkpoint_config = CheckpointConfig(config);
654+
// Also, we want to have items across 2 checkpoints.
655+
ASSERT_EQ(2, checkpoint_config.getMaxCheckpoints());
684656
createManager();
685657

686658
// Force the first checkpoint to be a disk one
@@ -716,15 +688,12 @@ TEST_P(CheckpointTest, DiskCheckpointStrictItemLimit) {
716688

717689
// Test the checkpoint cursor movement
718690
TEST_P(CheckpointTest, CursorMovement) {
719-
/* We want to have items across 2 checkpoints. Size down the default number
720-
of items to create a new checkpoint and recreate the manager */
721-
this->checkpoint_config =
722-
CheckpointConfig(DEFAULT_CHECKPOINT_PERIOD,
723-
MIN_CHECKPOINT_ITEMS,
724-
/*numCheckpoints*/ 2,
725-
/*itemBased*/ true,
726-
persistent() /*persistenceEnabled*/,
727-
CheckpointRemoval::Lazy);
691+
// Size down the default number of items to create a new checkpoint and
692+
// recreate the manager.
693+
config.setChkMaxItems(MIN_CHECKPOINT_ITEMS);
694+
checkpoint_config = CheckpointConfig(config);
695+
// Also, we want to have items across 2 checkpoints.
696+
ASSERT_EQ(2, checkpoint_config.getMaxCheckpoints());
728697
createManager();
729698

730699
/* Add items such that we have 1 full (max items as per config) checkpoint.
@@ -833,13 +802,11 @@ TEST_P(CheckpointTest, SeqnoAndHLCOrdering) {
833802
// configure with 1 checkpoint to ensure the time-based closing
834803
// does not split the items over many checkpoints and muddy the final
835804
// data checks.
836-
this->checkpoint_config =
837-
CheckpointConfig(DEFAULT_CHECKPOINT_PERIOD,
838-
n_threads * n_items,
839-
/*numCheckpoints*/ 2,
840-
/*itemBased*/ true,
841-
persistent() /*persistenceEnabled*/,
842-
CheckpointRemoval::Lazy);
805+
config.setChkMaxItems(n_threads * n_items);
806+
checkpoint_config = CheckpointConfig(config);
807+
// Also, we want to have items across 2 checkpoints.
808+
ASSERT_EQ(2, checkpoint_config.getMaxCheckpoints());
809+
843810
createManager();
844811

845812
std::vector<std::thread> threads;
@@ -1618,13 +1585,8 @@ TEST_P(CheckpointTest, DuplicateCheckpointCursor) {
16181585
TEST_P(CheckpointTest, DuplicateCheckpointCursorDifferentCheckpoints) {
16191586
// Size down the default number of items to create a new checkpoint and
16201587
// recreate the manager
1621-
this->checkpoint_config =
1622-
CheckpointConfig(DEFAULT_CHECKPOINT_PERIOD,
1623-
MIN_CHECKPOINT_ITEMS,
1624-
/*numCheckpoints*/ 2,
1625-
/*itemBased*/ true,
1626-
persistent() /*persistenceEnabled*/,
1627-
CheckpointRemoval::Lazy);
1588+
config.setChkMaxItems(MIN_CHECKPOINT_ITEMS);
1589+
checkpoint_config = CheckpointConfig(config);
16281590
createManager();
16291591

16301592
const auto& ckptList =
@@ -2259,13 +2221,8 @@ TEST_P(CheckpointTest, testDontExpelIfCursorAtMetadataItemWithSameSeqnoDisk) {
22592221
// then we will move the expel point backwards to the mutation
22602222
// (and possibly further).
22612223
void CheckpointTest::testDoNotExpelIfHaveSameSeqnoAfterMutation() {
2262-
this->checkpoint_config =
2263-
CheckpointConfig(DEFAULT_CHECKPOINT_PERIOD,
2264-
/*maxItemsInCheckpoint*/ 1,
2265-
/*numCheckpoints*/ 2,
2266-
/*itemBased*/ true,
2267-
persistent() /*persistenceEnabled*/,
2268-
CheckpointRemoval::Lazy);
2224+
config.setChkMaxItems(1);
2225+
checkpoint_config = CheckpointConfig(config);
22692226
createManager();
22702227

22712228
// Add a meta data operation
@@ -2948,7 +2905,7 @@ class EagerCheckpointDisposalTest : public CheckpointTest {
29482905
// Ensure the checkpoint config is set to that.
29492906
config.parseConfiguration("checkpoint_removal_mode=eager",
29502907
get_mock_server_api());
2951-
recreateCheckpointConfig();
2908+
checkpoint_config = CheckpointConfig(config);
29522909
}
29532910
};
29542911

‎engines/ep/tests/module_tests/checkpoint_test.h‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -33,10 +33,6 @@ class CheckpointTest : public VBucketTest {
3333
void TearDown() override;
3434

3535
protected:
36-
// Recreates the CheckpointConfig referenced by the CheckpointManager.
37-
// Used if the engine config has been changed and the CheckpointConfig
38-
// needs to be updated to match.
39-
void recreateCheckpointConfig();
4036
// Creates a new item with the given key and queues it into the checkpoint
4137
// manager.
4238
bool queueNewItem(const std::string& key);

‎engines/ep/tests/module_tests/vbucket_test.cc‎

Lines changed: 6 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -54,8 +54,11 @@ VBucketTestBase::VBucketTestBase(VBType vbType,
5454

5555
// MB-34453: Change sync_writes_max_allowed_replicas back to total
5656
// possible replicas given we want to still test against all replicas.
57-
config.parseConfiguration("sync_writes_max_allowed_replicas=3",
58-
get_mock_server_api());
57+
std::string confString = "sync_writes_max_allowed_replicas=3;bucket_type=";
58+
confString += (vbType == VBType::Persistent ? "persistent" : "ephemeral");
59+
// Also, a bunch of tests rely on max_checkpoints=2.
60+
confString += ";max_checkpoints=2";
61+
config.parseConfiguration(confString.c_str(), get_mock_server_api());
5962

6063
auto manifest = std::make_unique<Collections::VB::Manifest>(
6164
std::make_shared<Collections::Manager>());
@@ -107,17 +110,7 @@ VBucketTestBase::VBucketTestBase(VBType vbType,
107110
}
108111
}
109112

110-
checkpoint_config = CheckpointConfig(
111-
DEFAULT_CHECKPOINT_PERIOD,
112-
DEFAULT_CHECKPOINT_ITEMS,
113-
2 /*maxCheckpoints*/,
114-
true /*itemNumBasedNewCheckpoint*/,
115-
(vbType == VBType::Persistent ? true
116-
: false) /*persistenceEnabled*/,
117-
config.getCheckpointRemovalMode() ==
118-
::to_string(CheckpointRemoval::Eager)
119-
? CheckpointRemoval::Eager
120-
: CheckpointRemoval::Lazy);
113+
checkpoint_config = CheckpointConfig(config);
121114

122115
vbucket->checkpointManager = std::make_unique<MockCheckpointManager>(
123116
global_stats,

0 commit comments

Comments
 (0)