Add rowset id generator to FE and BE#1678

yiguolei · 2019-08-20T07:14:33Z

No description provided.

be/src/runtime/fe_based_rowset_id_generator.cpp

imay · 2019-08-20T08:00:44Z

be/src/runtime/fe_based_rowset_id_generator.cpp

+}
+
+OLAPStatus FeBasedRowsetIdGenerator::get_next_id(RowsetId* gen_rowset_id) {
+    std::lock_guard<std::mutex> l(_lock);


std::mutex is too heavy for get next id. In most cases, it is a only add operation

imay · 2019-08-20T08:03:28Z

fe/src/main/java/org/apache/doris/catalog/RowsetIdGenerator.java

+
+import com.google.common.collect.Maps;
+
+public class RowsetIdGenerator {


Can we name it a PersistedIdGernator? I think it can not only be used for RowsetId, but also be used for other id

Maybe it could not. because rowsetid generator could have many implementations, for example, we could scan all rowset id in storage engine at start time and then generate rowset it one by one. In this case it is not persistent.

For Id generator logic, we can use a class.
Then you can let RowsetIdGenerator contain it.

imay · 2019-08-20T08:06:01Z

gensrc/thrift/FrontendService.thrift

+// (start_rowset_id, end_rowset_id) not include start and end
+struct TGetRowsetIdResponse {
+    1: optional i64 start_rowset_id = -1
+    2: optional i64 end_rowset_id = -1


I prefer [start, end) with start_id and id_count

gaodayue · 2019-08-27T02:35:39Z

Can anyone explain what's the purpose of this PR and give a brief description of what's changed?

kangpinghuang · 2019-08-28T01:19:40Z

be/src/agent/agent_server.cpp

    vector<string> error_msgs;
    TStatusCode::type status_code = TStatusCode::OK;
-    return_value.__set_snapshot_version(PREFERRED_SNAPSHOT_VERSION);
+    int32_t return_snapshot_version = PREFERRED_SNAPSHOT_VERSION;


add some comment here to describe the version meaning.

kangpinghuang · 2019-08-28T01:21:54Z

be/src/olap/storage_engine.h

+
+    bool rowset_id_in_use(RowsetId& rowset_id) { return _rowset_id_generator->id_in_use(rowset_id); };
+
+    void release_rowset_id(RowsetId& rowset_id) { return _rowset_id_generator->release_id(rowset_id); };


add some comments here.

kangpinghuang · 2019-08-28T01:34:18Z

be/src/olap/rowset/rowset_id_generator.h

-    std::mutex _lock;
-    RowsetId _next_id = -1;
-    RowsetId _id_batch_end = -1;
+    virtual void release_id(RowsetId& rowset_id) = 0;


add comment to describe the api

kangpinghuang · 2019-08-28T01:34:39Z

be/src/olap/rowset/unique_rowset_id_generator.h

+    // generator a id according to data dir
+    // rowsetid is not globally unique, it is dir level
+    // it saves the batch end id into meta env
+    OLAPStatus next_id(RowsetId* rowset_id); 


Suggested change

OLAPStatus next_id(RowsetId* rowset_id);

OLAPStatus next_id(RowsetId* rowset_id) override;

kangpinghuang · 2019-08-28T01:34:54Z

be/src/olap/rowset/unique_rowset_id_generator.h

+    // it saves the batch end id into meta env
+    OLAPStatus next_id(RowsetId* rowset_id); 
+
+    bool id_in_use(RowsetId& rowset_id);


Suggested change

bool id_in_use(RowsetId& rowset_id);

bool id_in_use(const RowsetId& rowset_id) override;

kangpinghuang · 2019-08-28T02:07:56Z

be/src/olap/olap_common.h

+        lo = (id_version << 56) + (low & LOW_56_BITS);
+    }
+
+    std::string to_string() const {


I think you need to_string and serialize and deserialize api

to_string is ok, because it is a string value in pb.

kangpinghuang · 2019-08-28T02:10:56Z

be/src/olap/rowset/beta_rowset.cpp

@@ -26,7 +26,7 @@
 namespace doris {

 std::string BetaRowset::segment_file_path(const std::string& dir, RowsetId rowset_id, int segment_id) {


Suggested change

std::string BetaRowset::segment_file_path(const std::string& dir, RowsetId rowset_id, int segment_id) {

std::string BetaRowset::segment_file_path(const std::string& dir, const RowsetId& rowset_id, const int segment_id) {

be/src/olap/rowset/rowset_meta.h

kangpinghuang · 2019-08-28T02:23:39Z

be/src/olap/rowset/unique_rowset_id_generator.cpp

+    return _valid_rowset_ids.find(rowset_id) != _valid_rowset_ids.end();
+}
+
+void UniqueRowsetIdGenerator::release_id(RowsetId& rowset_id) {


Suggested change

void UniqueRowsetIdGenerator::release_id(RowsetId& rowset_id) {

void UniqueRowsetIdGenerator::release_id(const RowsetId& rowset_id) {

kangpinghuang · 2019-08-28T02:28:48Z

be/test/olap/rowset/unique_rowset_id_generator_test.cpp

+}
+
+
+TEST_F(UniqueRowsetIdGeneratorTest, GenerateIdTest) {


add a case to test BE restart situation.

morningman · 2019-08-28T13:54:59Z

be/src/service/doris_main.cpp

    // options
    doris::EngineOptions options;
    options.store_paths = paths;
+    options.backend_uid = doris::UniqueId();


Is backend_uid act like a random seed that will be generated every time BE started?
If so, I think the name is confused that make people think of it as the ID of a BE?
Maybe random_seed is better?

morningman · 2019-08-28T13:57:09Z

be/src/olap/olap_define.h

 static const uint16_t OLAP_STRING_MAX_LENGTH = 65535;

-static const int32_t PREFERRED_SNAPSHOT_VERSION = 2;
+static const int32_t PREFERRED_SNAPSHOT_VERSION = 3;


add some comment to explain the version 1 , 2, and 3?

imay · 2019-08-28T12:17:56Z

fe/src/main/java/org/apache/doris/task/SnapshotTask.java

        request.setVersion_hash(versionHash);
        request.setList_files(true);
-        request.setPreferred_snapshot_version(2);
+        request.setPreferred_snapshot_version(3);


Can make this a global variable? Its better not to write magic number

imay · 2019-08-28T13:38:15Z

be/test/olap/rowset/unique_rowset_id_generator_test.cpp

@@ -0,0 +1,111 @@
+// Licensed to the Apache Software Foundation (ASF) under one


you should add this file to run-ut.sh

imay · 2019-08-28T13:43:19Z

be/src/olap/storage_engine.h

    std::unique_ptr<TabletManager> _tablet_manager;
    std::unique_ptr<TxnManager> _txn_manager;

+    RowsetIdGenerator* _rowset_id_generator;


should delete it in destructor or use std::unique_ptr

imay · 2019-08-28T13:48:02Z

be/src/olap/rowset/unique_rowset_id_generator.cpp

+
+namespace doris {
+
+UniqueRowsetIdGenerator::UniqueRowsetIdGenerator(UniqueId backend_uid) :


Suggested change

UniqueRowsetIdGenerator::UniqueRowsetIdGenerator(UniqueId backend_uid) :

UniqueRowsetIdGenerator::UniqueRowsetIdGenerator(const UniqueId& backend_uid) :

imay · 2019-08-28T13:48:35Z

be/src/olap/rowset/unique_rowset_id_generator.cpp

+    _backend_uid(backend_uid), _inc_id(1) {
+}
+// generator a id according to data dir
+// rowsetid is not globally unique, it is dir level


I think rowset id is global unique

be/src/olap/rowset/segment_group.h

be/src/olap/rowset/rowset_meta_manager.h

be/src/olap/data_dir.cpp

be/src/olap/olap_common.h

yiguolei · 2019-09-02T02:22:45Z

#1717

Fix bug Compile success Build unit test successfully Rowsetid version error Add rowsetid gc logic Fix bug Add unique rowset id unit test

be/src/olap/storage_engine.h

be/src/olap/rowset/segment_group.h

imay

LGTM

)

* Fix BE UT failure introduced by apache#1526 in TabletSchemaCache (apache#1661) * Fix be ut * Revert "Fix BE UT failure introduced by apache#1526 in TabletSchemaCache (apache#1647)" This reverts commit bca5d32. * (selectdb-cloud) Modify debug checks for detached schema kv (apache#1678)

imay requested changes Aug 20, 2019

View reviewed changes

yiguolei force-pushed the master branch 2 times, most recently from f94cb37 to 7f14f10 Compare August 23, 2019 09:41

yiguolei force-pushed the master branch 2 times, most recently from efbc29d to 0d3988a Compare August 27, 2019 02:44

kangpinghuang requested changes Aug 28, 2019

View reviewed changes

yiguolei force-pushed the master branch from 4b2b669 to 43beefb Compare August 28, 2019 11:07

morningman reviewed Aug 28, 2019

View reviewed changes

imay requested changes Aug 28, 2019

View reviewed changes

yiguolei force-pushed the master branch 2 times, most recently from 594ee87 to 218e07e Compare August 29, 2019 07:41

yiguolei added 6 commits September 2, 2019 10:30

Add local unique rowset id generator

d5d8758

Fix bug Compile success Build unit test successfully Rowsetid version error Add rowsetid gc logic Fix bug Add unique rowset id unit test

Using low as rowset version

ee2f33e

Set rowset id to 0 when convert from old tablet

1af363d

Fix according to comment

77da432

Modify typo error according to review

2f00d2a

Add lock when check or release rowset id

00a9c4f

yiguolei force-pushed the master branch from 50fca36 to 00a9c4f Compare September 2, 2019 02:31

imay reviewed Sep 2, 2019

View reviewed changes

be/src/olap/storage_engine.h Outdated Show resolved Hide resolved

be/src/olap/rowset/segment_group.h Outdated Show resolved Hide resolved

be/src/olap/rowset/segment_group.h Show resolved Hide resolved

Using const & as parameter for rowsetid

f12dab0

imay approved these changes Sep 2, 2019

View reviewed changes

chaoyli merged commit 6f4feca into apache:master Sep 2, 2019

swjtu-zhanglei pushed a commit to swjtu-zhanglei/incubator-doris that referenced this pull request Jul 25, 2023

(selectdb-cloud) Modify debug checks for detached schema kv (apache#1678

5889a83

)


		import com.google.common.collect.Maps;

		public class RowsetIdGenerator {


		bool rowset_id_in_use(RowsetId& rowset_id) { return _rowset_id_generator->id_in_use(rowset_id); };

		void release_rowset_id(RowsetId& rowset_id) { return _rowset_id_generator->release_id(rowset_id); };

	OLAPStatus next_id(RowsetId* rowset_id);
	OLAPStatus next_id(RowsetId* rowset_id) override;

	bool id_in_use(RowsetId& rowset_id);
	bool id_in_use(const RowsetId& rowset_id) override;

		@@ -26,7 +26,7 @@
		namespace doris {

		std::string BetaRowset::segment_file_path(const std::string& dir, RowsetId rowset_id, int segment_id) {

	void UniqueRowsetIdGenerator::release_id(RowsetId& rowset_id) {
	void UniqueRowsetIdGenerator::release_id(const RowsetId& rowset_id) {

		@@ -0,0 +1,111 @@
		// Licensed to the Apache Software Foundation (ASF) under one


		namespace doris {

		UniqueRowsetIdGenerator::UniqueRowsetIdGenerator(UniqueId backend_uid) :

		}


		TEST_F(UniqueRowsetIdGeneratorTest, GenerateIdTest) {

Comments

Conversation

yiguolei commented Aug 20, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaodayue commented Aug 27, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!