Skip to content

Commit

Permalink
address comments & add encryption UT
Browse files Browse the repository at this point in the history
Signed-off-by: Lloyd-Pottiger <[email protected]>
  • Loading branch information
Lloyd-Pottiger committed Aug 28, 2024
1 parent 2751596 commit f11fac6
Show file tree
Hide file tree
Showing 12 changed files with 48 additions and 38 deletions.
8 changes: 7 additions & 1 deletion dbms/src/Storages/DeltaMerge/File/ColumnStat.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,10 @@ struct ColumnStat

std::optional<dtpb::VectorIndexFileProps> vector_index = std::nullopt;

#ifndef NDEBUG
// This field is only used for testing
String additional_data_for_test{};
#endif

dtpb::ColumnStat toProto() const
{
Expand All @@ -63,7 +66,9 @@ struct ColumnStat
if (vector_index.has_value())
stat.mutable_vector_index()->CopyFrom(vector_index.value());

#ifndef NDEBUG
stat.set_additional_data_for_test(additional_data_for_test);
#endif

return stat;
}
Expand All @@ -84,8 +89,9 @@ struct ColumnStat

if (proto.has_vector_index())
vector_index = proto.vector_index();

#ifndef NDEBUG
additional_data_for_test = proto.additional_data_for_test();
#endif
}

// @deprecated. New fields should be added via protobuf. Use `toProto` instead
Expand Down
2 changes: 1 addition & 1 deletion dbms/src/Storages/DeltaMerge/File/DMFile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ DMFilePtr DMFile::restore(
UInt64 page_id,
const String & parent_path,
const DMFileMeta::ReadMode & read_meta_mode,
UInt32 meta_version,
UInt64 meta_version,
KeyspaceID keyspace_id)
{
auto is_s3_file = S3::S3FilenameView::fromKeyWithPrefix(parent_path).isDataFile();
Expand Down
6 changes: 3 additions & 3 deletions dbms/src/Storages/DeltaMerge/File/DMFile.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include <Poco/File.h>
#include <Storages/DeltaMerge/DeltaMergeDefines.h>
#include <Storages/DeltaMerge/File/DMFileMetaV2.h>
#include <Storages/DeltaMerge/File/DMFileV3IncrementWriter_fwd.h>
#include <Storages/DeltaMerge/File/DMFile_fwd.h>
#include <Storages/FormatVersion.h>
#include <Storages/S3/S3Filename.h>
Expand All @@ -38,7 +39,6 @@ namespace tests
{
class DMFileTest;
class DMFileMetaV2Test;
class DMFileV3IncrementWriter;
class DMStoreForSegmentReadTaskTest;
} // namespace tests

Expand All @@ -62,7 +62,7 @@ class DMFile : private boost::noncopyable
UInt64 page_id,
const String & parent_path,
const DMFileMeta::ReadMode & read_meta_mode,
UInt32 meta_version = 0,
UInt64 meta_version = 0,
KeyspaceID keyspace_id = NullspaceID);

struct ListOptions
Expand Down Expand Up @@ -223,7 +223,7 @@ class DMFile : private boost::noncopyable
// Do not gc me.
String ngcPath() const;

String metav2Path(UInt32 meta_version) const { return subFilePath(DMFileMetaV2::metaFileName(meta_version)); }
String metav2Path(UInt64 meta_version) const { return subFilePath(DMFileMetaV2::metaFileName(meta_version)); }
UInt64 getReadFileSize(ColId col_id, const String & filename) const
{
return meta->getReadFileSize(col_id, filename);
Expand Down
6 changes: 3 additions & 3 deletions dbms/src/Storages/DeltaMerge/File/DMFileMetaV2.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ class DMFileMetaV2 : public DMFileMeta
KeyspaceID keyspace_id_,
DMConfigurationOpt configuration_,
DMFileFormat::Version format_version_,
UInt32 meta_version_)
UInt64 meta_version_)
: DMFileMeta(file_id_, parent_path_, status_, keyspace_id_, configuration_, format_version_)
, small_file_size_threshold(small_file_size_threshold_)
, merged_file_max_size(merged_file_max_size_)
Expand Down Expand Up @@ -80,7 +80,7 @@ class DMFileMetaV2 : public DMFileMeta
void finalize(WriteBuffer & buffer, const FileProviderPtr & file_provider, const WriteLimiterPtr & write_limiter)
override;
void read(const FileProviderPtr & file_provider, const DMFileMeta::ReadMode & read_meta_mode) override;
static String metaFileName(UInt32 meta_version = 0)
static String metaFileName(UInt64 meta_version = 0)
{
if (meta_version == 0)
return "meta";
Expand Down Expand Up @@ -111,7 +111,7 @@ class DMFileMetaV2 : public DMFileMeta

UInt64 small_file_size_threshold;
UInt64 merged_file_max_size;
UInt32 meta_version = 0; // Note: meta_version affects the output file name.
UInt64 meta_version = 0; // Note: meta_version affects the output file name.

private:
UInt64 getMergedFileSizeOfColumn(const MergedSubFileInfo & file_info) const;
Expand Down
4 changes: 2 additions & 2 deletions dbms/src/Storages/DeltaMerge/Remote/DataStore/DataStore.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ class IPreparedDMFileToken : boost::noncopyable
/**
* Restores into a DMFile object. This token will be kept valid when DMFile is valid.
*/
virtual DMFilePtr restore(DMFileMeta::ReadMode read_mode, UInt32 meta_version) = 0;
virtual DMFilePtr restore(DMFileMeta::ReadMode read_mode, UInt64 meta_version) = 0;

protected:
// These should be the required information for any kind of DataStore.
Expand Down Expand Up @@ -96,7 +96,7 @@ class IDataStore : boost::noncopyable
*
* When page_id is 0, will use its file_id as page_id.(Used by WN, RN can just use default value)
*/
virtual IPreparedDMFileTokenPtr prepareDMFile(const S3::DMFileOID & oid, UInt64 page_id = 0) = 0;
virtual IPreparedDMFileTokenPtr prepareDMFile(const S3::DMFileOID & oid, UInt64 page_id) = 0;

virtual IPreparedDMFileTokenPtr prepareDMFileByKey(const String & remote_key) = 0;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ static std::tuple<String, UInt64> parseDMFilePath(const String & path)
return std::tuple<String, UInt64>{parent_path, file_id};
}

DMFilePtr MockPreparedDMFileToken::restore(DMFileMeta::ReadMode read_mode, UInt32 meta_version)
DMFilePtr MockPreparedDMFileToken::restore(DMFileMeta::ReadMode read_mode, UInt64 meta_version)
{
auto [parent_path, file_id] = parseDMFilePath(path);
return DMFile::restore(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ class MockPreparedDMFileToken : public IPreparedDMFileToken

~MockPreparedDMFileToken() override = default;

DMFilePtr restore(DMFileMeta::ReadMode read_mode, UInt32 meta_version) override;
DMFilePtr restore(DMFileMeta::ReadMode read_mode, UInt64 meta_version) override;

private:
String path;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ IPreparedDMFileTokenPtr DataStoreS3::prepareDMFileByKey(const String & remote_ke
return prepareDMFile(oid, 0);
}

DMFilePtr S3PreparedDMFileToken::restore(DMFileMeta::ReadMode read_mode, UInt32 meta_version)
DMFilePtr S3PreparedDMFileToken::restore(DMFileMeta::ReadMode read_mode, UInt64 meta_version)
{
return DMFile::restore(
file_provider,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ class S3PreparedDMFileToken : public IPreparedDMFileToken

~S3PreparedDMFileToken() override = default;

DMFilePtr restore(DMFileMeta::ReadMode read_mode, UInt32 meta_version) override;
DMFilePtr restore(DMFileMeta::ReadMode read_mode, UInt64 meta_version) override;
};

} // namespace DB::DM::Remote
2 changes: 1 addition & 1 deletion dbms/src/Storages/DeltaMerge/Remote/Proto/remote.proto
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ message ColumnFileTiny {
message ColumnFileBig {
uint64 page_id = 1;
CheckpointInfo checkpoint_info = 2;
uint32 meta_version = 3; // Note: Only Stable cares about meta_version. ColumnFileBig does not care.
uint64 meta_version = 3; // Note: Only Stable cares about meta_version. ColumnFileBig does not care.

// TODO: We should better recalculate these fields from local DTFile.
uint64 valid_rows = 10;
Expand Down
47 changes: 25 additions & 22 deletions dbms/src/Storages/DeltaMerge/tests/gtest_dm_meta_version.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -395,10 +395,7 @@ class S3DMFile
INSTANTIATE_TEST_CASE_P( //
DMFileMetaVersion,
S3DMFile,
/* enable_encryption */ ::testing::Values(false));

// In this TiFlash version WN does not support encryption at all.
// See https://github.com/pingcap/tiflash/issues/8351
/* enable_encryption */ ::testing::Bool());

TEST_P(S3DMFile, Basic)
try
Expand All @@ -409,12 +406,14 @@ try
ASSERT_TRUE(dm_file->path().starts_with("s3://"));
ASSERT_EQ(0, dm_file->metaVersion());

auto token = dataStore()->prepareDMFile(S3::DMFileOID{
.store_id = store_id,
.keyspace_id = keyspace_id,
.table_id = table_id,
.file_id = 1,
});
auto token = dataStore()->prepareDMFile(
S3::DMFileOID{
.store_id = store_id,
.keyspace_id = keyspace_id,
.table_id = table_id,
.file_id = 1,
},
/* page_id= */ 0);
auto cn_dmf = token->restore(DMFileMeta::ReadMode::all(), 0);
ASSERT_EQ(0, cn_dmf->metaVersion());

Expand Down Expand Up @@ -446,12 +445,14 @@ try
iw->finalize();

// Read out meta version = 0
auto token = dataStore()->prepareDMFile(S3::DMFileOID{
.store_id = store_id,
.keyspace_id = keyspace_id,
.table_id = table_id,
.file_id = 1,
});
auto token = dataStore()->prepareDMFile(
S3::DMFileOID{
.store_id = store_id,
.keyspace_id = keyspace_id,
.table_id = table_id,
.file_id = 1,
},
/* page_id= */ 0);
auto cn_dmf = token->restore(DMFileMeta::ReadMode::all(), 0);
ASSERT_EQ(0, cn_dmf->metaVersion());
ASSERT_STREQ("", cn_dmf->meta->getColumnStats()[::DB::TiDBPkColumnID].additional_data_for_test.c_str());
Expand Down Expand Up @@ -497,12 +498,14 @@ try
}

// Read out meta version = 0
auto token = dataStore()->prepareDMFile(S3::DMFileOID{
.store_id = store_id,
.keyspace_id = keyspace_id,
.table_id = table_id,
.file_id = 1,
});
auto token = dataStore()->prepareDMFile(
S3::DMFileOID{
.store_id = store_id,
.keyspace_id = keyspace_id,
.table_id = table_id,
.file_id = 1,
},
/* page_id= */ 0);
auto cn_dmf = token->restore(DMFileMeta::ReadMode::all(), 0);
ASSERT_EQ(0, cn_dmf->metaVersion());
ASSERT_STREQ("", cn_dmf->meta->getColumnStats()[::DB::TiDBPkColumnID].additional_data_for_test.c_str());
Expand Down
3 changes: 2 additions & 1 deletion dbms/src/Storages/S3/tests/gtest_s3file.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,8 @@ class S3FileTest

DMFilePtr restoreDMFile(const DMFileOID & oid)
{
return data_store->prepareDMFile(oid)->restore(DMFileMeta::ReadMode::all(), /* meta_version= */ 0);
return data_store->prepareDMFile(oid, /* page_id= */ 0)
->restore(DMFileMeta::ReadMode::all(), /* meta_version= */ 0);
}

LoggerPtr log;
Expand Down

0 comments on commit f11fac6

Please sign in to comment.