Separate data storage abstraction for MergeTree by KochetovNicolai · Pull Request #36555 · ClickHouse/ClickHouse

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

    }

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


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

src/Storages/MergeTree/DataPartsExchange.cpp

        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

        = MutationsInterpreter::MutationKind::MutationKindEnum::MUTATE_UNKNOWN;

-    VolumePtr single_disk_volume;
+    //VolumePtr single_disk_volume;


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

src/Storages/MergeTree/MergeTreeDataPartCompact.cpp

 {
    checkConsistencyBase();
-    String path = getFullRelativePath();
+    // String path = getRelativePath();


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

src/Storages/StorageMergeTree.cpp

    {
-        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


-    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

                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

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

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):

Uh oh!

Choose a reason for hiding this comment

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants