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.

Sign up for GitHub

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

Jump to bottom

feat: initial changes for compaction_time_window field support #1083

Merged
merged 7 commits into from
Apr 6, 2023
Merged

feat: initial changes for compaction_time_window field support #1083

merged 7 commits into from
Apr 6, 2023

Conversation

etolbakov
Copy link
Collaborator

@etolbakov etolbakov commented Feb 25, 2023 •
edited
Loading

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

  • add a field compaction_time_window in TableOptions.
  • persist the inferred time window to table manifest to avoid calculating it everytime.

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.

Related issue link

#1055

@killme2008
Copy link
Contributor

hi, @etolbakov , are u still working on this PR?

@etolbakov
Copy link
Collaborator Author

hi @killme2008
yeah, sorry it took more time than I initially though. We are in touch with @v0y4g3r on it. I think I'll be able to create a PR this weekend if that's ok?

@killme2008
Copy link
Contributor

hi @killme2008 yeah, sorry it took more time than I initially though. We are in touch with @v0y4g3r on it. I think I'll be able to create a PR this weekend if that's ok?

I got it. Just a reminder. Go at your own pace. Thank you.

@etolbakov etolbakov changed the base branch from develop to admin March 27, 2023 09:37
@etolbakov etolbakov changed the base branch from admin to develop March 27, 2023 09:37
@etolbakov etolbakov marked this pull request as ready for review March 30, 2023 08:08
@etolbakov
Copy link
Collaborator Author

Hi @v0y4g3r
Could you please take a look at the changes when you have time?

@v0y4g3r
Copy link
Contributor

v0y4g3r commented Mar 30, 2023

Hi @v0y4g3r Could you please take a look at the changes when you have time?

Sure, I will review this PR tommorow

v0y4g3r
v0y4g3r approved these changes Apr 3, 2023
View reviewed changes
Copy link
Contributor

@v0y4g3r v0y4g3r left a comment

Choose a reason for hiding this comment

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

Generally looks good to me. I will resolve the confilicts with upstream so that it can be merged.

src/table/src/requests.rs Show resolved Hide resolved
@waynexia waynexia self-requested a review April 3, 2023 06:08
@v0y4g3r
Copy link
Contributor

v0y4g3r commented Apr 3, 2023 •
edited
Loading

@etolbakov I rebased feat/compaction_time_window onto latest develop, please take another look at the changes to ensure I didn't miss something. Also, persisting inferred compaction time window is not incoperated in this PR so please add a todo in CompactionTaskImpl::write_manifest_and_apply in case we forget it.

@codecov
Copy link

codecov bot commented Apr 3, 2023 •
edited
Loading

Codecov Report

Merging #1083 (a5b3e40) into develop (5848f27) will decrease coverage by 0.36%.
The diff coverage is 89.42%.

@@             Coverage Diff             @@
##           develop    #1083      +/-   ##
===========================================
- Coverage    85.80%   85.44%   -0.36%     
===========================================
  Files          503      506       +3     
  Lines        76227    76863     +636     
===========================================
+ Hits         65403    65675     +272     
- Misses       10824    11188     +364

waynexia
waynexia reviewed Apr 3, 2023
View reviewed changes
Copy link
Member

@waynexia waynexia left a comment

Choose a reason for hiding this comment

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

Mostly looks good to me 👍

src/table/src/requests.rs Outdated Show resolved Hide resolved
@etolbakov
Copy link
Collaborator Author

@v0y4g3r I've checked the changes after the rebase, LGTM.
I've added the TODO as well

@v0y4g3r v0y4g3r requested a review from waynexia April 5, 2023 10:16
waynexia
waynexia approved these changes Apr 6, 2023
View reviewed changes
Copy link
Member

@waynexia waynexia left a comment

Choose a reason for hiding this comment

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

Looking great, thanks @etolbakov

@waynexia waynexia merged commit 59f7630 into GreptimeTeam:develop Apr 6, 2023
@etolbakov etolbakov deleted the feat/compaction_time_window branch May 7, 2023 18:51
paomian pushed a commit to paomian/greptimedb that referenced this pull request Oct 19, 2023
@etolbakov @paomian
feat: initial changes for compaction_time_window field support (Grept…
9ae8877 
…imeTeam#1083)

* feat(compaction_time_window): initial changes for compaction_time_window field support

* feat(compaction_time_window): move PickerContext creation

* feat(compaction_time_window): update region descriptor, fix formatting

* feat(compaction_time_window): add minor enhancements

* feat(compaction_time_window): fix failing test

* feat(compaction_time_window):  return an error instead silently skip for the user provided compaction_time_window

* feat(compaction_time_window): add TODO reminder
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@v0y4g3r v0y4g3r v0y4g3r approved these changes

@waynexia waynexia waynexia approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@etolbakov @killme2008 @v0y4g3r @waynexia