ISSUES-2581 fix SELECT with sequential consistency #2863

VadimPlh · 2018-08-14T10:01:45Z

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

…ES-2581

alexey-milovidov · 2018-09-01T17:57:15Z

dbms/src/Storages/StorageReplicatedMergeTree.h

@@ -403,6 +403,14 @@ class StorageReplicatedMergeTree : public ext::shared_ptr_helper<StorageReplicat
      */
    bool queueTask();

+    std::unordered_map<String, Int64> getMaxBlocksWithQuorum(const std::unordered_map<String, String> & last_parts);
+
+    /// Get parts, wich were wrote with quorum.


Don't understand what does this function return.

alexey-milovidov · 2018-09-01T17:58:50Z

dbms/src/Storages/StorageReplicatedMergeTree.h

+    std::unordered_map<String, String> getLastPartsWithQuorum(const String & last_parts_str);
+
+    /// Rewrite last parts with quorum
+    String rewriteLastParts(const String & old_last_parts, const String & new_part_name);


Need more descriptive comment.
Is it about a data in ZooKeeper? (no)

alexey-milovidov · 2018-09-01T17:59:16Z

dbms/src/Storages/StorageReplicatedMergeTree.h

@@ -403,6 +403,14 @@ class StorageReplicatedMergeTree : public ext::shared_ptr_helper<StorageReplicat
      */
    bool queueTask();

+    std::unordered_map<String, Int64> getMaxBlocksWithQuorum(const std::unordered_map<String, String> & last_parts);


Missing comment. Is the return value keyed by partition id?

alexey-milovidov · 2018-09-01T17:59:37Z

dbms/src/Storages/StorageReplicatedMergeTree.cpp


-        if (last_part.empty())  /// If no part has been written with quorum.
+	if (last_parts.empty())  /// If no part has been written with quorum.


alexey-milovidov · 2018-09-01T18:00:25Z

dbms/src/Storages/MergeTree/MergeTreeDataSelectExecutor.cpp

@@ -137,11 +137,11 @@ BlockInputStreams MergeTreeDataSelectExecutor::read(
    const Context & context,
    const size_t max_block_size,
    const unsigned num_streams,
-    Int64 max_block_number_to_read) const
+    const std::unordered_map<String, Int64> & max_blocks_number_to_read) const


Why don't typedef (using)?

alexey-milovidov · 2018-09-01T18:01:40Z

dbms/src/Storages/MergeTree/StorageFromMergeTreeDataPart.h

@@ -27,7 +27,7 @@ class StorageFromMergeTreeDataPart : public ext::shared_ptr_helper<StorageFromMe
        unsigned num_streams) override
    {
        return MergeTreeDataSelectExecutor(part->storage).readFromParts(
-            {part}, column_names, query_info, context, max_block_size, num_streams, 0);
+            {part}, column_names, query_info, context, max_block_size, num_streams, std::unordered_map<String, Int64>());


BTW, You can write {} as a function argument.

alexey-milovidov · 2018-09-01T18:01:57Z

dbms/src/Storages/StorageMergeTree.cpp

@@ -115,7 +115,7 @@ BlockInputStreams StorageMergeTree::read(
    const unsigned num_streams)
 {
    checkQueryProcessingStage(processed_stage, context);
-    return reader.read(column_names, query_info, context, max_block_size, num_streams, 0);
+    return reader.read(column_names, query_info, context, max_block_size, num_streams, std::unordered_map<String, Int64>());


ztlpn

Some cosmetic changes are necessary. Also missing tests.

ztlpn · 2018-09-05T12:00:13Z

dbms/src/Storages/StorageReplicatedMergeTree.cpp

+                added_parts[partition_id] = part_name;
+            }
+        }
+        catch (Exception e)


Exception used in non-exceptional situation (also, error code is not checked and exception is caught by value). Better use checkString function from IO/ReadHelpers.h

ztlpn · 2018-09-05T12:01:29Z

dbms/src/Storages/StorageReplicatedMergeTree.cpp

+    {
+        try
+        {
+            in >> "parts_count " >> count >> "\n";


If we have two serialization versions of the same struct, better have an explicit version string (see e.g. MergeTreeDataPartChecksums class.

ztlpn · 2018-09-05T12:04:44Z

dbms/src/Storages/StorageReplicatedMergeTree.cpp

+}
+
+
+String StorageReplicatedMergeTree::rewriteAddedParts(const String & old_added_parts_str, const String & new_part_name)


Better to split logic responsible for serialization of the struct and logic responsible for its update. That is, there should be a generic write() method.

ztlpn · 2018-09-05T12:07:50Z

dbms/src/Storages/StorageReplicatedMergeTree.cpp

+}
+
+
+PartsNames StorageReplicatedMergeTree::getAddedPartsWithQuorum(const String & parts_str)


Should be called read... or parse..., not get...

ztlpn · 2018-09-05T12:11:33Z

dbms/src/Storages/StorageReplicatedMergeTree.h

@@ -403,6 +403,20 @@ class StorageReplicatedMergeTree : public ext::shared_ptr_helper<StorageReplicat
      */
    bool queueTask();

+    using MaxAddedBlocks = std::unordered_map<String, Int64>;
+    using PartsNames = std::unordered_map<String, String>;


No information about the meaning of keys. Also, MaxAddedBlocks is too specific. I would call these PartitionToBlockNum and PartitionToPartName.

ztlpn · 2018-09-05T12:14:38Z

dbms/src/Storages/StorageReplicatedMergeTree.cpp

-            auto part_info = MergeTreePartInfo::fromPartName(last_part, data.format_version);
-            max_block_number_to_read = part_info.max_block;
+            auto added_parts = getAddedPartsWithQuorum(added_parts_str);
+            for (auto & added_part : added_parts)


Why non-const reference?

ztlpn · 2018-09-05T12:15:34Z

dbms/src/Storages/StorageReplicatedMergeTree.h

+
+    /// Rewrite data with inserted partiton with quorum.
+    /// This is added new partition name in data for ZooKeeper.
+    String rewriteAddedParts(const String & old_added_parts_str, const String & new_added_part_name);


I think these methods deserve to be separated into another class. StorageReplicatedMergeTree is already overloaded.

ztlpn · 2018-09-05T12:19:11Z

dbms/src/Storages/MergeTree/MergeTreeDataSelectExecutor.cpp

@@ -267,7 +267,8 @@ BlockInputStreams MergeTreeDataSelectExecutor::readFromParts(
                    part->minmax_idx.parallelogram, data.minmax_idx_column_types))
                continue;

-            if (max_block_number_to_read && part->info.max_block > max_block_number_to_read)
+            if (!max_blocks_number_to_read.empty() &&  max_blocks_number_to_read.find(part->info.partition_id) != max_blocks_number_to_read.end()


empty() check is unnecessary. The call to at() is unnecessary too (can use the results of find()).

…nto ISSUES-2581

VadimPE and others added 2 commits August 14, 2018 12:57

ISSUES-2581 fix SELECT with select_sequential_consistency

3c0f6ce

Update MergeTreeDataSelectExecutor.cpp

537a8f0

VadimPlh mentioned this pull request Aug 14, 2018

Sequential select bug after current partition change #2581

Closed

VadimPlh changed the title ~~ISSUES-2581 fix SELECT with select_sequential_consistency~~ ISSUES-2581 fix SELECT with sequential consistency Aug 14, 2018

VadimPE and others added 9 commits August 14, 2018 13:24

ISSUES-2581 fix build

a37ad50

Merge branch 'ISSUES-2581' of github.com:VadimPE/ClickHouse into ISSU…

74aded6

…ES-2581

ISSUES-2581 add check when replica do not have part added with quorum

c8e2506

Merge branch 'master' into ISSUES-2581

3598f39

Update StorageReplicatedMergeTree.cpp

854192e

Update MergeTreeDataSelectExecutor.h

306c1b0

Update MergeTreeDataSelectExecutor.cpp

b96d15f

Update MergeTreeDataSelectExecutor.cpp

5f7117e

Update MergeTreeDataSelectExecutor.h

79a089a

VadimPlh requested review from alexey-milovidov and ztlpn August 21, 2018 14:48

VadimPlh added 2 commits August 21, 2018 17:53

Update MergeTreeDataSelectExecutor.cpp

e9954d3

Merge branch 'master' into ISSUES-2581

a246b91

alesapin assigned ztlpn Aug 28, 2018

alexey-milovidov reviewed Sep 1, 2018

View reviewed changes

ISSUES-2581 add comments, and fix

6e78f57

ztlpn suggested changes Sep 5, 2018

View reviewed changes

VadimPE and others added 3 commits September 17, 2018 15:28

Add new class for write new parts

5defbad

Merge branch 'master' into ISSUES-2581

204e1d4

Update MergeTreeDataSelectExecutor.cpp

4dee095

VadimPE and others added 25 commits September 21, 2018 18:15

del include

17aa035

Update ReplicatedMergeTreeQuorumAddedParts.h

8c413e3

ISSUES-2581 get pointer to max_blocls

28102bc

ISSUES-2581 fix bug with new partition

d4a56af

ISSUES-2581 add tests

cc2686c

ISSUES-2581 fix name

4db6eab

Update 00732_quorum_insert_zookeeper.reference

8c5be3a

Update 00732_quorum_insert_zookeeper.sql

9399287

Update StorageReplicatedMergeTree.cpp

25b100c

Update 00732_quorum_insert_zookeeper.sql

6b2bfd2

Update 00732_quorum_insert_zookeeper.sql

517619d

ISSUES-2581 add test

1c51407

ISSUES-2581 add test

58c2bd3

Update 00732_quorum_insert_lost_part_and_alive_part_zookeeper.reference

9b1271a

Update 00732_quorum_insert_simple_test_1_parts_zookeeper.reference

5898a22

Update 00732_quorum_insert_simple_test_2_parts_zookeeper.reference

a013dce

fix tests

4d6da00

Merge branch 'ISSUES-2581' of https://github.com/VadimPE/ClickHouse i…

fca4a7b

…nto ISSUES-2581

fix test

52deb39

ISSUES-2581

868c1bd

fix name

070eae2

fix addedparts

4a93d3b

ad test

4a0ca4c

rename partition -> part

451114c

fix bug with merge part, wich us inserted with quorum

f2db463

ztlpn approved these changes Oct 18, 2018

View reviewed changes

fix codestyle

31794b7

alexey-milovidov merged commit 98c2ed5 into ClickHouse:master Oct 22, 2018

alexey-milovidov added a commit that referenced this pull request Oct 23, 2018

Fixed error #2863

bac6b6f

blinkov unassigned ztlpn Sep 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ISSUES-2581 fix SELECT with sequential consistency #2863

ISSUES-2581 fix SELECT with sequential consistency #2863

VadimPlh commented Aug 14, 2018

alexey-milovidov Sep 1, 2018

alexey-milovidov Sep 1, 2018

alexey-milovidov Sep 1, 2018 •

edited

Loading

alexey-milovidov Sep 1, 2018

alexey-milovidov Sep 1, 2018

alexey-milovidov Sep 1, 2018

alexey-milovidov Sep 1, 2018 •

edited

Loading

alexey-milovidov Sep 1, 2018

ztlpn left a comment

ztlpn Sep 5, 2018

ztlpn Sep 5, 2018

ztlpn Sep 5, 2018

ztlpn Sep 5, 2018

ztlpn Sep 5, 2018

ztlpn Sep 5, 2018

ztlpn Sep 5, 2018

ztlpn Sep 5, 2018


		if (last_part.empty()) /// If no part has been written with quorum.
		if (last_parts.empty()) /// If no part has been written with quorum.

		}


		String StorageReplicatedMergeTree::rewriteAddedParts(const String & old_added_parts_str, const String & new_part_name)

		}


		PartsNames StorageReplicatedMergeTree::getAddedPartsWithQuorum(const String & parts_str)

ISSUES-2581 fix SELECT with sequential consistency #2863

ISSUES-2581 fix SELECT with sequential consistency #2863

Conversation

VadimPlh commented Aug 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexey-milovidov Sep 1, 2018 • edited Loading

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

alexey-milovidov Sep 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ztlpn left a 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

alexey-milovidov Sep 1, 2018 •

edited

Loading

alexey-milovidov Sep 1, 2018 •

edited

Loading