-
Notifications
You must be signed in to change notification settings - Fork 409
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] PageStorage: introduce ExternalMap to manage DTFile lifetime #4000
Conversation
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
002c7f0
to
d291bc6
Compare
/run-unit-test |
85c502a
to
0d1ec4b
Compare
TODO:
|
/run-unit-test |
1 similar comment
/run-unit-test |
Coverage for changed files
Coverage summary
full coverage report (for internal network access only) |
552da1a
to
332b70e
Compare
: iter->second->getEntry(last_sequence + 1)); | ||
entry) | ||
{ | ||
// copy the entry to be ref |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seem like there still have some GC problem?
If ori_page_id
has been removed by gc. then the entry used ref_pageid
can't find data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is an unsolved problem that after implementing the "REF", when should we remove the entry from the BlobStore(spacemap)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #4061
b6692b8
to
a2f7724
Compare
Signed-off-by: JaySon-Huang <tshent@qq.com>
Signed-off-by: JaySon-Huang <tshent@qq.com>
Signed-off-by: JaySon-Huang <tshent@qq.com>
Signed-off-by: JaySon-Huang <tshent@qq.com>
Signed-off-by: JaySon-Huang <tshent@qq.com>
104a150
to
fb55812
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with some comments
bool ExternalMap::createExternal(PageId page_id, UInt64 sequence) | ||
{ | ||
auto [iter, new_inserted] = external_table_directory.insert(std::make_pair(page_id, ExternalPageHolder(page_id, sequence))); | ||
if (unlikely(!new_inserted)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we roll back it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing needs to be roll back here.
If new_inserted == false
, then iter
is an iterator that points to the existing item. Nothing gets inserted or changed so nothing needs to be roll back.
> cat a.cpp
#include <map>
#include <iostream>
using namespace std;
struct A
{
int a, b;
A(int a_, int b_): a(a_), b(b_) {}
};
int main()
{
map<int, A> m;
{
auto [iter, new_inserted] = m.insert(std::make_pair(1, A{1, 2}));
cout << "new: " << new_inserted << ". " << iter->first << ":" << iter->second.a << "," << iter->second.b << endl;
}
{
auto [iter, new_inserted] = m.insert(std::make_pair(1, A{3, 4}));
cout << "new: " << new_inserted << ". " << iter->first << ":" << iter->second.a << "," << iter->second.b << endl;
}
return 0;
}
> g++ a.cpp -std=c++17 && ./a.out
new: 1. 1:1,2
new: 0. 1:1,2
// Try to apply ref on external pages | ||
if (!external_directory.tryCreateRef(record.page_id, record.ori_page_id, record.version.sequence)) | ||
{ | ||
// LOG_FMT_WARNING(log, "Trying to add ref from {} to non-exist {} with seq={}", record.page_id, record.ori_page_id, record.version.sequence); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should not return false
? or should we throw a exception here, cause it looks like a logic error.
{ | ||
table_directory.erase(iter); | ||
// put after ref, must be sth wrong!!! | ||
// LOG_FMT_WARNING(log, "Trying to add ref from {} to non-exist {} with seq={}", record.page_id, record.ori_page_id, record.version.sequence); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep this log.
Signed-off-by: JaySon-Huang <tshent@qq.com>
Signed-off-by: JaySon-Huang <tshent@qq.com>
do it by another design #4174 |
What problem does this PR solve?
Issue Number: close #4017
Problem Summary:
What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note