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

txn: add watermark #132

Merged
merged 14 commits into from Jun 14, 2022
Merged

txn: add watermark #132

merged 14 commits into from Jun 14, 2022

Conversation

GanZiheng
Copy link
Contributor

@GanZiheng GanZiheng marked this pull request as draft June 1, 2022 09:57
@GanZiheng
Copy link
Contributor Author

Will rebase onto master once #129 merged.

@GanZiheng
Copy link
Contributor Author

A little bit weird, we will get thread sanitizer error if we use yatp. For example, https://github.com/tikv/agatedb/runs/6705381794?check_suite_focus=true.

@GanZiheng GanZiheng marked this pull request as ready for review June 2, 2022 10:25
@GanZiheng
Copy link
Contributor Author

So I use standard library to spawn threads.

@skyzh
Copy link
Member

skyzh commented Jun 3, 2022

https://github.com/tikv/agatedb/runs/6705381794?check_suite_focus=true

This is a known issue of crossbeam-epoch. If we want to use crossbeam channel in AgateDB, we will need to disable thread sanitizer check (or allow it to ignore crossbeam-related structures).

Few projects will run tests with sanitizers. I think we can periodically check address / leak / thread sanitizer when things go wrong, and simply remove sanitizer checks from CI.

@GanZiheng
Copy link
Contributor Author

https://github.com/tikv/agatedb/runs/6705381794?check_suite_focus=true

This is a known issue of crossbeam-epoch. If we want to use crossbeam channel in AgateDB, we will need to disable thread sanitizer check (or allow it to ignore crossbeam-related structures).

Few projects will run tests with sanitizers. I think we can periodically check address / leak / thread sanitizer when things go wrong, and simply remove sanitizer checks from CI.

But when I use standard library to spawn thread, the issue disappears. Sort of strange...

@skyzh
Copy link
Member

skyzh commented Jun 5, 2022

But when I use standard library to spawn thread, the issue disappears. Sort of strange...

Maybe by accident or some magic 🤪

@zhangjinpeng87
Copy link
Member

@GanZiheng Please fix conflicts.

Signed-off-by: GanZiheng <ganziheng98@gmail.com>
Signed-off-by: GanZiheng <ganziheng98@gmail.com>
Signed-off-by: GanZiheng <ganziheng98@gmail.com>
Signed-off-by: GanZiheng <ganziheng98@gmail.com>
GanZiheng and others added 3 commits June 6, 2022 17:16
Signed-off-by: GanZiheng <ganziheng98@gmail.com>
Signed-off-by: GanZiheng <ganziheng98@gmail.com>
@codecov
Copy link

codecov bot commented Jun 7, 2022

Codecov Report

Merging #132 (72e4742) into master (628064b) will increase coverage by 1.94%.
The diff coverage is 84.58%.

@@            Coverage Diff             @@
##           master     #132      +/-   ##
==========================================
+ Coverage   83.65%   85.60%   +1.94%     
==========================================
  Files          35       36       +1     
  Lines        6230     6753     +523     
==========================================
+ Hits         5212     5781     +569     
+ Misses       1018      972      -46     

src/watermark.rs Outdated Show resolved Hide resolved
Signed-off-by: GanZiheng <ganziheng98@gmail.com>
src/watermark.rs Outdated Show resolved Hide resolved
GanZiheng and others added 2 commits June 8, 2022 16:31
Signed-off-by: GanZiheng <ganziheng98@gmail.com>
src/closer.rs Outdated Show resolved Hide resolved
src/watermark.rs Outdated Show resolved Hide resolved
Signed-off-by: GanZiheng <ganziheng98@gmail.com>
Signed-off-by: GanZiheng <ganziheng98@gmail.com>
Copy link
Member

@zhangjinpeng87 zhangjinpeng87 left a comment

Choose a reason for hiding this comment

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

LGTM

@GanZiheng GanZiheng mentioned this pull request Jun 9, 2022
28 tasks
Copy link
Member

@Connor1996 Connor1996 left a comment

Choose a reason for hiding this comment

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

WaterMarkInner can be avoided, though LGTM

@Connor1996
Copy link
Member

/merge

@Connor1996 Connor1996 merged commit 5c62ccb into tikv:master Jun 14, 2022
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.

None yet

4 participants