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

Issue 3908 - Send asynchronous compaction commits to new commit lambda from compaction task #4131

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from

Conversation

rtjd6554
Copy link
Collaborator

@rtjd6554 rtjd6554 commented Jan 24, 2025

Make sure you have checked all steps below.

Issue

Tests

  • My PR adds the following tests OR does not need testing for this extremely good reason:
    • Updated CompactionTaskCommitTest, ECSCompactionTaskRunnerLocalStackIT

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it, or I have linked to a
    separate issue for that below.
  • If I have added or removed any dependencies from the project, I have updated the NOTICES file.

@patchwork01 patchwork01 marked this pull request as ready for review January 24, 2025 12:15
@patchwork01 patchwork01 assigned gaffer01 and unassigned gaffer01 Jan 24, 2025
@@ -152,6 +152,11 @@ sleeper.table.compaction.job.id.assignment.commit.async=true
# instance property.
sleeper.table.compaction.job.commit.async=true

# If true, asynchronous commits will be batched if they are enabled. The commits will be gathered into
# a larger transaction per Sleeper table. If false, the commits will sent directly to the commit
# lambda.
Copy link
Member

Choose a reason for hiding this comment

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

This would be clearer as:

"This property affects whether commits of compaction jobs are batched before being sent to the state store commit queue to be applied by the committer lambda. If this property is true and asynchronous commits are enabled then commits of compactions will be batched. If this property is false and asynchronous commits are enabled then commits of compactions will not be batched and will be sent directly to the committer lambda."

@@ -1462,6 +1462,10 @@ sleeper.default.compaction.job.id.assignment.commit.async=true
# finished jobs directly to the state store.
sleeper.default.compaction.job.commit.async=true

# This is the default for whether the asynchronous compaction commits will be sent to the batcher if
# enabled.
Copy link
Member

Choose a reason for hiding this comment

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

This would be clearer as:

"This property is the default for whether commits of compaction jobs are batched before being sent to the state store commit queue to be applied by the committer lambda. If this property is true and asynchronous commits are enabled then commits of compactions will be batched. If this property is false and asynchronous commits are enabled then commits of compactions will not be batched and will be sent directly to the committer lambda. This property can be overridden for individual tables."

@@ -294,6 +294,12 @@ public interface TableDefaultProperty {
.defaultValue("true")
.validationPredicate(SleeperPropertyValueUtils::isTrueOrFalse)
.propertyGroup(InstancePropertyGroup.TABLE_PROPERTY_DEFAULT).build();
UserDefinedInstanceProperty DEFAULT_COMPACTION_JOB_ASYNC_BATCHING = Index.propertyBuilder("sleeper.default.compaction.job.async.commit.batching")
.description("This is the default for whether the asynchronous compaction commits will be sent " +
"to the batcher if enabled.")
Copy link
Member

Choose a reason for hiding this comment

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

See comment above.

.defaultProperty(DEFAULT_COMPACTION_JOB_ASYNC_BATCHING)
.description("If true, asynchronous commits will be batched if they are enabled. The commits will be " +
"gathered into a larger transaction per Sleeper table. If false, the commits will sent directly " +
"to the commit lambda. ")
Copy link
Member

Choose a reason for hiding this comment

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

See comment above.

@gaffer01
Copy link
Member

Approved pending above comments.

@gaffer01 gaffer01 assigned rtjd6554 and unassigned gaffer01 Jan 24, 2025
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.

Send asynchronous compaction commits to new commit lambda from compaction task
3 participants