Separate data storage abstraction for MergeTree #36555

KochetovNicolai · 2022-04-22T17:33:10Z

Changelog category (leave one):

Not for changelog (changelog entry is not required)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

.

amosbird · 2022-04-26T14:48:15Z

src/Storages/MergeTree/IMergeTreeDataPart.cpp

-            ErrorCodes::LOGICAL_ERROR);
+        throw Exception(
+            ErrorCodes::LOGICAL_ERROR,
+            "Attempt to store uninitialized MinMax index for part {}. This is a bug.",


trailing period.

src/Storages/MergeTree/DataPartStorageOnDisk.cpp

alesapin · 2022-06-21T10:46:48Z

src/Storages/MergeTree/IDataPartStorage.h

+class TemporaryFileOnDisk;
+
+/// This is an abstraction of storage for data part files.
+/// Generally, it contains read-only methods from IDisk.


outdated comment? I see a lot of write methods...

alesapin · 2022-06-21T10:47:06Z

src/Storages/MergeTree/IDataPartStorage.h

+    virtual ~IDataPartStorage() = default;
+
+    /// Methods to get path components of a data part.
+    virtual std::string getFullPath() const = 0;      /// '/var/lib/clickhouse/data/database/table/moving/all_1_5_1'


👍 for example in comment.

alesapin · 2022-06-21T10:47:28Z

src/Storages/MergeTree/IDataPartStorage.h

+/// Generally, it contains read-only methods from IDisk.
+class IDataPartStorage
+{
+private:


Suggested change

private:

alesapin · 2022-06-21T10:50:50Z

src/Storages/MergeTree/DataPartsExchange.cpp

@@ -268,7 +268,7 @@ MergeTreeData::DataPart::Checksums Service::sendPartFromDisk(
        checksums.files[file_name] = {};
    }

-    auto disk = part->volume->getDisk();
+    //auto disk = part->volume->getDisk();


alesapin · 2022-06-21T10:50:56Z

src/Storages/MergeTree/DataPartsExchange.cpp

@@ -294,22 +294,26 @@ MergeTreeData::DataPart::Checksums Service::sendPartFromDisk(
    {
        String file_name = it.first;

-        String path = fs::path(part->getFullRelativePath()) / file_name;
+        //String path = fs::path(part->getRelativePath()) / file_name;


alesapin · 2022-06-21T11:27:42Z

src/Storages/MergeTree/MutateTask.cpp

@@ -653,10 +654,11 @@ struct MutationContext
    MutationsInterpreter::MutationKind::MutationKindEnum mutation_kind
        = MutationsInterpreter::MutationKind::MutationKindEnum::MUTATE_UNKNOWN;

-    VolumePtr single_disk_volume;
+    //VolumePtr single_disk_volume;


alesapin · 2022-06-21T11:28:08Z

src/Storages/MergeTree/MergeTreeDataPartCompact.cpp

@@ -137,7 +139,7 @@ bool MergeTreeDataPartCompact::hasColumnFiles(const NameAndTypePair & column) co
 void MergeTreeDataPartCompact::checkConsistency(bool require_part_metadata) const
 {
    checkConsistencyBase();
-    String path = getFullRelativePath();
+    // String path = getRelativePath();


alesapin · 2022-06-21T11:28:36Z

src/Storages/StorageMergeTree.cpp

@@ -1739,29 +1739,25 @@ CheckResults StorageMergeTree::checkData(const ASTPtr & query, ContextPtr local_

    for (auto & part : data_parts)
    {
-        auto disk = part->volume->getDisk();
-        String part_path = part->getFullRelativePath();
+        //auto disk = part->volume->getDisk();


alesapin · 2022-06-21T11:28:51Z

src/Storages/MergeTree/MutateTask.cpp

@@ -1479,8 +1497,8 @@ bool MutateTask::prepare()
    ctx->new_data_part->setSerializationInfos(new_infos);
    ctx->new_data_part->partition.assign(ctx->source_part->partition);

-    ctx->disk = ctx->new_data_part->volume->getDisk();
-    ctx->new_part_tmp_path = ctx->new_data_part->getFullRelativePath();
+    // ctx->disk = ctx->new_data_part->volume->getDisk();


alesapin · 2022-06-21T11:29:09Z

src/Storages/MergeTree/MutateTask.cpp

        {
            if (ctx->files_to_skip.contains(it->name()))
                continue;

-            String destination = ctx->new_part_tmp_path;
+            String destination; // = ctx->new_part_tmp_path;


Suggested change

String destination; // = ctx->new_part_tmp_path;

String destination;

…in-part-volumes" This reverts commit cc6fdfe, reversing changes made to da578ec.

tavplubix · 2022-06-30T13:27:51Z

src/Storages/MergeTree/IMergeTreeDataPart.cpp

-{
-    if (relative_path.empty())
-        throw Exception("Part relative_path cannot be empty. It's bug.", ErrorCodes::LOGICAL_ERROR);
+// String IMergeTreeDataPart::getFullPath() const


Please remove commented code

KochetovNicolai added 10 commits April 5, 2022 19:12

Try refactor something (1)

5a1392a

Part 2

5cbec37

Part 3

bc3e1ec

Part 4

7c4b288

Part 5

76870ad

Part 6.

bcbab2e

Part 7

9133e39

Part 8

8c00692

Fix mark cache.

1805d60

More fixes

e7e6125

amosbird reviewed Apr 26, 2022

View reviewed changes

Merge branch 'master' into refactor-something-in-part-volumes

e44af67

robot-ch-test-poll added the pr-not-for-changelog This PR should not be mentioned in the changelog label Apr 26, 2022

KochetovNicolai and others added 17 commits May 3, 2022 15:48

Fixing build.

4a20e4f

Merge branch 'master' into refactor-something-in-part-volumes

3509519

Fixing build.

49e613a

Fix some tests.

fc3645a

Fix some more tests.

b3d3830

Fixing style.

5574a3c

Fixing spaces.

d6735f0

Merge branch 'master' into refactor-something-in-part-volumes

89c5855

Fixing build.

25c8a43

Fix some tests.

3f67abc

Merge branch 'master' into refactor-something-in-part-volumes

678d978

Merge branch 'master' into refactor-something-in-part-volumes

5bc9b32

Fix test_encrypted_disk

a274bec

Fix test_encrypted_disk

2ee349f

Merge branch 'master' into refactor-something-in-part-volumes

2a9a63a

Fixing build

25b9990

Merge branch 'master' into refactor-something-in-part-volumes

50801e4

alesapin added 2 commits June 19, 2022 17:12

Fix double negative

08b5dd0

Fix strange diff

119ce90

alesapin reviewed Jun 20, 2022

View reviewed changes

src/Storages/MergeTree/DataPartStorageOnDisk.cpp Outdated Show resolved Hide resolved

src/Storages/MergeTree/DataPartStorageOnDisk.cpp Outdated Show resolved Hide resolved

KochetovNicolai and others added 4 commits June 20, 2022 13:07

Fix more tests.

ef8b967

Merge branch 'master' into refactor-something-in-part-volumes

7452d04

Continure resolving conflicts.

39b02d4

Cleanup.

dccf90b

KochetovNicolai marked this pull request as ready for review June 20, 2022 18:18

KochetovNicolai and others added 4 commits June 21, 2022 07:26

Fix

3429a5e

Fix style

5232c17

Do not create empty folder for in-memory parts.

e45d552

Merge branch 'master' into refactor-something-in-part-volumes

1e8c9ec

alesapin self-assigned this Jun 21, 2022

Fixing build.

46c4fd6

alesapin approved these changes Jun 21, 2022

View reviewed changes

alesapin changed the title ~~Refactor something in part volumes~~ Separate data storage abstraction for MergeTree Jun 21, 2022

KochetovNicolai added 2 commits June 21, 2022 12:31

Add some comments. Remove some commented code.

854d148

Roll back some changes from IPartMetadataManager

dcf6e0d

KochetovNicolai merged commit cc6fdfe into master Jun 22, 2022

KochetovNicolai deleted the refactor-something-in-part-volumes branch June 22, 2022 09:13

alesapin added a commit that referenced this pull request Jun 27, 2022

Revert "Merge pull request #36555 from ClickHouse/refactor-something-…

1c31692

…in-part-volumes" This reverts commit cc6fdfe, reversing changes made to da578ec.

tavplubix reviewed Jun 30, 2022

View reviewed changes

tavplubix mentioned this pull request Jun 30, 2022

BC check: Cannot quickly remove directory ... by removing files #38629

Closed

azat mentioned this pull request Jul 8, 2022

Fix fsync_part_directory for fetches #38993

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate data storage abstraction for MergeTree #36555

Separate data storage abstraction for MergeTree #36555

KochetovNicolai commented Apr 22, 2022

amosbird Apr 26, 2022

alesapin Jun 21, 2022

alesapin Jun 21, 2022

alesapin Jun 21, 2022

alesapin Jun 21, 2022

alesapin Jun 21, 2022

alesapin Jun 21, 2022

alesapin Jun 21, 2022

alesapin Jun 21, 2022

alesapin Jun 21, 2022

alesapin Jun 21, 2022

tavplubix Jun 30, 2022

	String destination; // = ctx->new_part_tmp_path;
	String destination;

Separate data storage abstraction for MergeTree #36555

Separate data storage abstraction for MergeTree #36555

Conversation

KochetovNicolai commented Apr 22, 2022

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

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