Skip to content

Raptor background jobs#13849

Merged
jessesleeping merged 3 commits intoprestodb:masterfrom
kewang1024:Raptor_background_jobs
Dec 20, 2019
Merged

Raptor background jobs#13849
jessesleeping merged 3 commits intoprestodb:masterfrom
kewang1024:Raptor_background_jobs

Conversation

@kewang1024
Copy link
Collaborator

@kewang1024 kewang1024 commented Dec 11, 2019

== Raptor ==
- Add delta delete support in Raptor. If table property "table_supports_delta_delete" is set to true
when creating a Raptor table, DELETE query will write down "tombstones" of deleted data instead 
of rewriting files immediately. 

@kewang1024
Copy link
Collaborator Author

Given the last PR is still in process, this PR still have some commits from last PR.
But we can first review the logic and coding style in
Enable delta compaction and organization
Add priority to Compaction process
Add delta in reassign process

I will create a new PR after the last PR is merged.
@jessesleeping @highker

Thanks!

@kewang1024 kewang1024 force-pushed the Raptor_background_jobs branch from fb2256b to a23872b Compare December 11, 2019 19:30
Copy link

@highker highker left a comment

Choose a reason for hiding this comment

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

some comments

@kewang1024 kewang1024 force-pushed the Raptor_background_jobs branch from a23872b to 3a6009a Compare December 12, 2019 07:22
@kewang1024 kewang1024 requested a review from highker December 13, 2019 20:20
Copy link
Contributor

@jessesleeping jessesleeping left a comment

Choose a reason for hiding this comment

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

Reviewed commit "Enable delta compaction and organization". Only minor comments.

Copy link
Contributor

@jessesleeping jessesleeping left a comment

Choose a reason for hiding this comment

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

Reviewed commit "Add priority to Compaction process". Some general comments besides the inline minor comments:

  • Instead of using int to indicate priority, I suggest we use an enum which has DELTA_COMPACTION and NORMAL_COMPACTION for now. It will be more readable.
  • Can you add test case to verify if the behavior of ShardOrganizer is as expected?

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of adding 2 levels of callback, can you extract the for loop into a function and call it at onSuccess() and onFailure() below?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But that would add duplicate code, Let's also confirm with James before we make this change

Copy link
Contributor

Choose a reason for hiding this comment

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

Why would this create duplicate code?

Copy link
Contributor

@jessesleeping jessesleeping left a comment

Choose a reason for hiding this comment

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

Commit "Add delta in reassign process" LGTM.

@kewang1024
Copy link
Collaborator Author

  • DELTA_COMPACTION

But currently I also add how many delta we have as the priority, so that the OrganizationSet that has the most delta will be compacted first, so the number couldn't be abstracted as ENUM for now

@kewang1024 kewang1024 force-pushed the Raptor_background_jobs branch from 3a6009a to 4ff6874 Compare December 15, 2019 21:43
Copy link

@highker highker left a comment

Choose a reason for hiding this comment

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

Could you rebase?

@kewang1024 kewang1024 force-pushed the Raptor_background_jobs branch from 4ff6874 to 8934760 Compare December 18, 2019 23:39
@kewang1024
Copy link
Collaborator Author

the third PR of #13248

@kewang1024 kewang1024 force-pushed the Raptor_background_jobs branch 5 times, most recently from 042494c to 45c54c3 Compare December 19, 2019 01:26
Copy link

@highker highker left a comment

Choose a reason for hiding this comment

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

nits only. Will leave to @jessesleeping for the final approval and merge.

@kewang1024 kewang1024 force-pushed the Raptor_background_jobs branch from 45c54c3 to f69836b Compare December 20, 2019 18:57
@jessesleeping jessesleeping merged commit 5f781d3 into prestodb:master Dec 20, 2019
@aweisberg aweisberg mentioned this pull request Jan 17, 2020
7 tasks
@caithagoras caithagoras mentioned this pull request Jan 22, 2020
6 tasks
@kewang1024 kewang1024 deleted the Raptor_background_jobs branch March 12, 2023 20:24
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