-
Notifications
You must be signed in to change notification settings - Fork 49
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
Implementing Finalize Backfill Hook for All Backfila Clients #358
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,8 @@ package app.cash.backfila.client.fixedset | |
import app.cash.backfila.client.spi.BackfilaParametersOperator | ||
import app.cash.backfila.client.spi.BackfillOperator | ||
import app.cash.backfila.client.spi.parametersToBytes | ||
import app.cash.backfila.protos.clientservice.FinalizeBackfillRequest | ||
import app.cash.backfila.protos.clientservice.FinalizeBackfillResponse | ||
import app.cash.backfila.protos.clientservice.GetNextBatchRangeRequest | ||
import app.cash.backfila.protos.clientservice.GetNextBatchRangeResponse | ||
import app.cash.backfila.protos.clientservice.KeyRange | ||
|
@@ -89,4 +91,12 @@ class FixedSetBackfillOperator<Param : Any>( | |
.end(end.toString().encodeUtf8()) | ||
.build() | ||
} | ||
|
||
override fun finalizeBackfill(request: FinalizeBackfillRequest): FinalizeBackfillResponse { | ||
val config = parametersOperator.constructBackfillConfig(request) | ||
backfill.finalize(config) | ||
|
||
return FinalizeBackfillResponse.Builder() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the |
||
.build() | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ package app.cash.backfila.client.s3 | |
|
||
import app.cash.backfila.client.Backfill | ||
import app.cash.backfila.client.BackfillConfig | ||
import app.cash.backfila.client.FinalizeBackfillConfig | ||
import app.cash.backfila.client.PrepareBackfillConfig | ||
import app.cash.backfila.client.s3.record.RecordStrategy | ||
|
||
|
@@ -48,4 +49,9 @@ abstract class S3DatasourceBackfill<R : Any, P : Any> : Backfill { | |
* Produces records from the S3 file. | ||
*/ | ||
abstract val recordStrategy: RecordStrategy<R> | ||
|
||
/** | ||
* Override this to do any work after the backfill completes. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we need to update the comment a bit: Does that make sense? |
||
*/ | ||
open fun finalize(config: FinalizeBackfillConfig<P>) {} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,3 +13,9 @@ data class PrepareBackfillConfig<Param : Any>( | |
val parameters: Param, | ||
val dryRun: Boolean, | ||
) | ||
|
||
data class FinalizeBackfillConfig<Param : Any>( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we need better names and/or structure for these. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PartitionlessBackfillConfig? Or WholeBackfillConfig? This is config that is meant to represent the whole run. WholeRunBackfillConfig? Also add the accessor above. And probably an accessor to Prepare here too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm ok with it as is |
||
val parameters: Param, | ||
val backfillId: String, | ||
val dryRun: Boolean, | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,9 +21,9 @@ internal class BackfillRegisteredParameters @Inject constructor( | |
private val queryFactory: Query.Factory, | ||
) : HibernateBackfill<DbRegisteredBackfill, Id<DbRegisteredBackfill>, NoParameters>() { | ||
|
||
override fun runOne(id: Id<DbRegisteredBackfill>, config: BackfillConfig<NoParameters>) { | ||
override fun runOne(pkey: Id<DbRegisteredBackfill>, config: BackfillConfig<NoParameters>) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. undo the changes to this class |
||
transacter.transaction { session -> | ||
val backfill = session.load(id) | ||
val backfill = session.load(pkey) | ||
if (backfill.parameters.size == backfill.parameterNames().size) { | ||
return@transaction | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI This is a test only implementation.