pingcap / tiflash

The analytical engine for TiDB and TiDB Cloud. Try free: https://tidbcloud.com/free-trial
https://docs.pingcap.com/tidb/stable/tiflash-overview
Apache License 2.0
941 stars 409 forks source link

Remove embedded member function for gtest in `dbms/src/Storages/Page` #4562

Open Lloyd-Pottiger opened 2 years ago

Lloyd-Pottiger commented 2 years ago

Enhancement

There are some embedded member function in the directory dbms/src/Storages/Page like PS::V3::PageStorageImpl

#ifndef NDEBUG
    // Just for tests, refactor them out later
    DB::PageStorage::SnapshotPtr getSnapshot() { return getSnapshot(""); }
    DB::PageEntry getEntry(PageId page_id) { return getEntryImpl(TEST_NAMESPACE_ID, page_id, nullptr); }
    DB::Page read(PageId page_id) { return readImpl(TEST_NAMESPACE_ID, page_id, nullptr, nullptr); }
    PageMap read(const std::vector<PageId> & page_ids) { return readImpl(TEST_NAMESPACE_ID, page_ids, nullptr, nullptr); }
    void read(const std::vector<PageId> & page_ids, const PageHandler & handler) { return readImpl(TEST_NAMESPACE_ID, page_ids, handler, nullptr, nullptr); }
    PageMap read(const std::vector<PageReadFields> & page_fields) { return readImpl(TEST_NAMESPACE_ID, page_fields, nullptr, nullptr); }
#endif

which is just for tests. This is not a elegant code style, and we need to remove them.

SteNicholas commented 2 years ago

@Lloyd-Pottiger, could you assign this ticket to me? I have interest to do the first issue.

Lloyd-Pottiger commented 2 years ago

@Lloyd-Pottiger, could you assign this ticket to me? I have interest to do the first issue.

Sure, may be you can talk about your ideas about the solutions?

SteNicholas commented 2 years ago

@Lloyd-Pottiger, the solution is that removes embedded member functions for gtest in dbms/src/Storages/Page and adds embedded member functions to a common basic TestSuite. Right?

Lloyd-Pottiger commented 2 years ago

@Lloyd-Pottiger, the solution is that removes embedded member functions for gtest in dbms/src/Storages/Page and adds embedded member functions to a common basic TestSuite. Right?

@SteNicholas Those functions are only needed for gtest, so we can just remove them. However, in gtests, we used them, and we should keep the normal usage of gtests and can not delete gtests. May we can change the function we called in gtests line by line which is a huge project, or add marco like #define read(x) read(TEST_NAMESPACE_ID, x) which could not work for every case, and so on. Looking forward to fancy ideas!