Skip to content
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

[FLASH-206] Refactor Process About Region Data #36

Merged
merged 12 commits into from
Apr 8, 2019
Merged

[FLASH-206] Refactor Process About Region Data #36

merged 12 commits into from
Apr 8, 2019

Conversation

solotzg
Copy link
Contributor

@solotzg solotzg commented Apr 4, 2019

  • move the process about region data, such as write|default|lock cf to a new class RegionData.
  • Decode the TiKV key & value at first.
  • Optimize and refactor some code.

@solotzg
Copy link
Contributor Author

solotzg commented Apr 6, 2019

/run-integration-tests

@solotzg solotzg requested a review from flowbehappy April 7, 2019 02:19
@flowbehappy
Copy link
Contributor

Maybe we should stop making the contributor agreement? It is a private project and those features won't likely be merged back to CH later.

auto next(RegionWriteCFDataTrait::Keys * keys = nullptr)
{
if (!found)
throw Exception(String() + "table: " + DB::toString(expected_table_id) + " is not found", ErrorCodes::LOGICAL_ERROR);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we need String() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that's a mistake

}

private:
RegionPtr store;
std::unique_lock<std::shared_mutex> lock;

bool found;
RegionWriteCFData::Data::iterator write_cf_data_it;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe storing RegionWriteCFData::Map is better?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's also a choice. if so, we need to store the expected table id because removeDataByWriteIt needs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is kind of weird that caching an iterator here, since it won't change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, I just use this iterator as a pointer to seek TableID and RegionWriteCFData::Map

Copy link
Contributor

@flowbehappy flowbehappy Apr 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is better to cache TableID and RegionWriteCFData::Map & directly here, because an iterator is potentially means changing. And it should run faster since we can skip one pointer reference. But any way, it is your choice.

@zyguan
Copy link
Contributor

zyguan commented Apr 7, 2019

/rebuild

2 similar comments
@solotzg
Copy link
Contributor Author

solotzg commented Apr 8, 2019

/rebuild

@zyguan
Copy link
Contributor

zyguan commented Apr 8, 2019

/rebuild

@solotzg
Copy link
Contributor Author

solotzg commented Apr 8, 2019

/run-integration-tests

@flowbehappy
Copy link
Contributor

LGTM

@solotzg solotzg merged commit 272ccb7 into pingcap:master Apr 8, 2019
@solotzg solotzg deleted the FLASH-206 branch April 8, 2019 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants