[DNM] Raft integration and TMT storage review #32

innerr · 2019-04-02T12:14:33Z

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

flowbehappy · 2019-04-03T03:13:45Z

dbms/src/Raft/RaftService.cpp

@@ -25,6 +25,7 @@ RaftService::RaftService(const std::string & address_, DB::Context & db_context_

 RaftService::~RaftService()
 {
+    // REVIEW: can we remove this mutex?


Will there be any upcoming commits to fix this? Or just put some questions in codebase?

Because I think messages like this should put somewhere else like Jira instead of here.

All that messages will be addressed(PRs/issues should be created) after online-video discussion, before that nothing need to do

…mt-review

solotzg · 2019-04-08T09:47:22Z

dbms/src/Storages/Transaction/RegionMeta.cpp

@@ -42,6 +42,8 @@ RegionMeta RegionMeta::deserialize(ReadBuffer & buf)
    return RegionMeta(peer, region, apply_state, applied_term, pending_remove);
 }

+// REVIEW: lock? be carefull, can be easily deadlock.
+//  or use member `const RegionID region_id`
 RegionID RegionMeta::regionId() const { return region.id(); }


it should return region_id. I added the member region_id but forgot to change this part.

solotzg · 2019-04-08T13:36:13Z

dbms/src/Storages/Transaction/applySnapshot.cpp

@@ -44,6 +48,8 @@ void applySnapshot(KVStorePtr kvstore, RequestReader read, Context * context)
            auto cf_name = data.cf();
            auto key = TiKVKey();
            auto value = TiKVValue();
+            // REVIEW: the batch inserting logic is OK, but the calling stack is weird.
+            //   May be we should just do lock action on each node-inserting, it's also fast when lock contention is low.


By now, we don't need a specific function to deal with batch inserting. I implement this function just there was a TODO.

solotzg · 2019-04-08T13:37:52Z

dbms/src/Storages/Transaction/RegionPersister.cpp

@@ -21,6 +23,7 @@ void RegionPersister::persist(const RegionPtr & region, enginepb::CommandRespons
    // Support only on thread persist.
    std::lock_guard<std::mutex> lock(mutex);

+    // REVIEW: can we just region->resetPersistParm(void)?


I store the value first because it may be changed during doPersist.

solotzg · 2019-04-08T13:38:19Z

dbms/src/Storages/Transaction/RegionMeta.h

@@ -110,7 +112,9 @@ class RegionMeta

 // When we create a region peer, we should initialize its log term/index > 0,
 // so that we can force the follower peer to sync the snapshot first.
+// REVIEW: why initialize term = 5?


ask menglong, this is a magic number.

solotzg · 2019-04-08T13:42:40Z

dbms/src/Storages/Transaction/RegionMeta.cpp

@@ -176,6 +181,7 @@ void RegionMeta::doSetPendingRemove() { pending_remove = true; }
 void RegionMeta::waitIndex(UInt64 index)
 {
    std::unique_lock<std::mutex> lock(mutex);
+    // REVIEW: should we lock inside the closure function?


std::condition_variable itself will lock.

solotzg · 2019-04-08T13:44:10Z

dbms/src/Storages/Transaction/RegionMeta.cpp

@@ -127,9 +130,11 @@ enginepb::CommandResponse RegionMeta::toCommandResponse() const

 RegionMeta::RegionMeta(RegionMeta && rhs) : region_id(rhs.regionId())
 {
+    // REVIEW: lock rhs


maybe no need. I just added a lock in case

solotzg · 2019-04-08T13:47:09Z

dbms/src/Storages/Transaction/RegionMeta.cpp

    peer = std::move(rhs.peer);
    region = std::move(rhs.region);
    apply_state = std::move(rhs.apply_state);
+    // REVIEW: set rhs.* to init state


even if a region is at initial state, its data is not empty. since we've decided to move it, it should not be used any more.

solotzg · 2019-04-08T13:47:31Z

dbms/src/Storages/Transaction/applySnapshot.cpp

@@ -15,12 +15,16 @@ void applySnapshot(KVStorePtr kvstore, RequestReader read, Context * context)

    enginepb::SnapshotRequest request;
    auto ok = read(&request);
+    // REVIEW: use two 'throw' here, 'not ok' and 'no state'


solotzg · 2019-04-08T13:47:40Z

dbms/src/Storages/Transaction/RegionPersister.cpp

@@ -40,6 +43,7 @@ void RegionPersister::doPersist(const RegionPtr & region, enginepb::CommandRespo
    }

    MemoryWriteBuffer buffer;
+    // REVIEW: add some warning log here if region is too large


solotzg · 2019-04-08T13:47:56Z

dbms/src/Storages/Transaction/RegionPersister.cpp

@@ -11,6 +11,8 @@ extern const int LOGICAL_ERROR;

 void RegionPersister::drop(RegionID region_id)
 {
+    // REVIEW: need mutex?


@flowbehappy

solotzg · 2019-04-08T13:49:43Z

dbms/src/Storages/Transaction/RegionMeta.h

@@ -70,6 +70,7 @@ class RegionMeta
    bool isPendingRemove() const;
    void setPendingRemove();

+    // REVIEW: assign is a better name


can be considered

solotzg · 2019-04-08T13:50:06Z

dbms/src/Storages/Transaction/RegionMeta.h

@@ -94,6 +95,7 @@ class RegionMeta
    void doSetApplied(UInt64 index, UInt64 term);

 private:
+    // REVIEW: we should make sure all these member are deepcopy, (eg: region.peers())


solotzg · 2019-04-09T05:00:24Z

dbms/src/Storages/Transaction/RegionMeta.cpp

@@ -225,6 +232,7 @@ void RegionMeta::execChangePeer(

    switch (change_peer_request.change_type())
    {
+        // REVIEW: throws when meet `AddNode`?


any raft command is sent to all peer. we should discard those we don't need.

solotzg · 2019-04-09T05:05:04Z

dbms/src/Storages/Transaction/KVStore.h

@@ -58,6 +58,7 @@ class KVStore final : private boost::noncopyable

    std::atomic<Timepoint> last_try_persist_time = Clock::now();

+    // REVIEW: the task_mutex looks should be one mutex per region than one big lock in kvstore


because of operation like split and merge, we can not regard region as independent one.

solotzg · 2019-04-09T05:10:59Z

dbms/src/Storages/Transaction/KVStore.cpp

@@ -272,6 +282,9 @@ void KVStore::removeRegion(RegionID region_id, Context * context)
    }

    region_persister.drop(region_id);
+
+    // REVIEW: if process crushed here, then when the process start again, the region_table will not find this region, is it OK?


In region_table, we don't store the range of table to disk. At any time, we should ensure that the range in region_table should contains the one in region_persister to avoid losing data. After restarted, we can recompute the range of table.

solotzg · 2019-04-09T05:12:40Z

dbms/src/Storages/Transaction/KVStore.cpp

        }
        else
        {
+            // REVIEW: is the persisting order OK?


updateRegion can only add info of range, can't change

solotzg · 2019-04-09T05:13:50Z

dbms/src/Storages/Transaction/KVStore.cpp

@@ -160,6 +163,7 @@ void KVStore::onServiceCommand(const enginepb::CommandRequestBatch & cmds, RaftC
                    auto [it, ok] = regions.emplace(new_region->id(), new_region);
                    if (!ok)
                    {
+                        // REVIEW: do we need to compare the old one and the new one?


because index of the splitted one is definitely init state.

solotzg · 2019-04-09T05:33:53Z

dbms/src/Storages/Transaction/KVStore.cpp

@@ -82,11 +83,13 @@ void KVStore::onSnapshot(RegionPtr new_region, Context * context)

        if (new_region->isPendingRemove())
        {
+            // REVIEW: here we remove the region in persister, then below the region is added to persister again


It's a kind of ABA problem. I have avoid the wrong process about selecting in applySnapshot and MergeTreeDataSelectExecutor::read.
However one region may appear in two store, because rngine may crush during send raft log which contains ChangePeer cmd. It will bring disaster to our learner read process.

solotzg · 2019-04-09T05:34:41Z

dbms/src/Storages/Transaction/KVStore.cpp

@@ -66,6 +66,7 @@ void KVStore::onSnapshot(RegionPtr new_region, Context * context)
        {
            LOG_DEBUG(log, "KVStore::onSnapshot: previous " << old_region->toString(true) << " ; new " << new_region->toString(true));

+            // REVIEW: in what case this can happen? rngine crushed?


not sure. just for idempotent.

zyguan · 2019-04-11T12:59:40Z

/rebuild

flowbehappy · 2019-04-23T08:21:09Z

dbms/src/Storages/Page/PageFile.cpp

    fd = ::open(path.c_str(), flags, 0666);
    if (-1 == fd)
    {
        ProfileEvents::increment(ProfileEvents::FileOpenFailed);
+        // REVIEW: this seems not right


It is a bug. Should be
if constexpr (!must_exist)

RaftService/KVStore review

a9655e9

innerr changed the title ~~RaftService / KVStore review~~ [DNM] RaftService / KVStore review Apr 2, 2019

flowbehappy reviewed Apr 3, 2019

View reviewed changes

innerr added 5 commits April 3, 2019 17:29

Reviewing StorageMergeTree

2da8045

StorageMergeTree review

8484ef5

RegionPersister review. Reviwing Region

90f9b9b

Merge branch 'raft-tmt-review' of github.com:pingcap/tics into raft-t…

4e773d7

…mt-review

RegionMeta review

5ab2fec

solotzg reviewed Apr 8, 2019

View reviewed changes

innerr changed the title ~~[DNM] RaftService / KVStore review~~ [DNM] Raft integration and TMT storage review Apr 8, 2019

solotzg reviewed Apr 8, 2019

View reviewed changes

solotzg reviewed Apr 9, 2019

View reviewed changes

innerr added 2 commits April 9, 2019 14:39

Merge master

655103c

Region reviewing

2198099

innerr added 7 commits April 10, 2019 19:16

Region reviewing

c00e57c

client-c (aka: PD/Region Client) review

496948f

Region review

ed6eddf

Region review

0282d76

RegionData review

fc4f0bc

Codec/RegionTable reviewing

b97a48a

Exceptions reviewing

ee01c1b

innerr added 15 commits April 12, 2019 15:27

Merge branch 'master' of github.com:pingcap/tics into raft-tmt-review

f03a03e

RegionTable reviewing

b8ce52f

Merge branch 'master' of github.com:pingcap/tics into raft-tmt-review

b941e9c

RegionTable review

e2b8404

Reviewing

de75e71

Codec review

7960935

HashCheckHelper review

2803f01

TiDB.* and TiKVKeyValue review

a9adb5b

Update TODO

377265c

RegionBlockReader review

0cf3ce3

PageStorage reviewing

4ac613e

PageFile reviewing

4de0525

PageStorage reviewing

5612116

PageStorage review

a618601

DataStreams / Storages review

68fe31f

flowbehappy reviewed Apr 23, 2019

View reviewed changes

innerr added 2 commits April 24, 2019 15:49

Group review done

eaa4a65

Review the reviewing

c89e110

solotzg mentioned this pull request Apr 25, 2019

[FLASH-231] Refactor Structure And Module #52

Merged

solotzg closed this Dec 17, 2019

solotzg deleted the raft-tmt-review branch December 17, 2019 04:32

yibin87 mentioned this pull request Feb 22, 2024

ASAN/TSan failed compute test case #8285

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DNM] Raft integration and TMT storage review #32

[DNM] Raft integration and TMT storage review #32

innerr commented Apr 2, 2019

flowbehappy Apr 3, 2019

flowbehappy Apr 3, 2019

innerr Apr 3, 2019

solotzg Apr 8, 2019

solotzg Apr 8, 2019

solotzg Apr 8, 2019

solotzg Apr 8, 2019

solotzg Apr 8, 2019

solotzg Apr 8, 2019

solotzg Apr 8, 2019

solotzg Apr 8, 2019

solotzg Apr 8, 2019

solotzg Apr 8, 2019

solotzg Apr 8, 2019

solotzg Apr 8, 2019

solotzg Apr 8, 2019

solotzg Apr 9, 2019

solotzg Apr 9, 2019

solotzg Apr 9, 2019

solotzg Apr 9, 2019

solotzg Apr 9, 2019 •

edited

Loading

solotzg Apr 9, 2019

solotzg Apr 9, 2019

zyguan commented Apr 11, 2019

flowbehappy Apr 23, 2019

		@@ -58,6 +58,7 @@ class KVStore final : private boost::noncopyable

		std::atomic<Timepoint> last_try_persist_time = Clock::now();

		// REVIEW: the task_mutex looks should be one mutex per region than one big lock in kvstore

[DNM] Raft integration and TMT storage review #32

[DNM] Raft integration and TMT storage review #32

Conversation

innerr commented Apr 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solotzg Apr 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyguan commented Apr 11, 2019

Choose a reason for hiding this comment

solotzg Apr 9, 2019 •

edited

Loading