streamingccl: add EXPIRATION WINDOW option to ALTER VIRTUAL CLUSTER #117776
Conversation
Epic: none Release note: none
|
This change is |
msbutler
force-pushed
the
butler-rebased-alter-tenant-producer
branch
4 times, most recently
from
e5c7a5b
to
c411be4
Compare
msbutler
changed the title
msbutler
requested review from
dt and
stevendanna
and removed request for
a team
| md.Progress.GetStreamReplication().Expiration = newExpiration | ||
| ju.UpdateProgress(md.Progress) |
There was a problem hiding this comment.
This might be more than we want to chew in this PR, but I'm mildly of the opinion that the builtin really should be saving the LastCheckinTime rather than the expiration.
| // We don't allow the user to specify the producer job expirationWindow during | ||
| // replication creation from the destination cluster, as this job setting | ||
| // should only be set from the producer cluster. | ||
| return nil, nil, nil, false, errors.New("cannot specify expirationWindow option while starting a physical replication stream") |
There was a problem hiding this comment.
Maybe: s/expirationWindow option/the EXPIRATION WINDOW option/
There was a problem hiding this comment.
Not necessarily for this PR, but I wonder if we should allow them to set the expiration window here and then the expiration set here would be used on the producer job we created post cutover, failing back to the retention window if it is is unset. But, I suppose then there would be confusion about why that doesn't also set the expiration window on the producer job on the source.
| if !alterTenantStmt.Options.IsDefault() { | ||
| return alterTenantSetReplication(ctx, p.InternalSQLTxn(), jobRegistry, options, tenInfo) | ||
| } |
There was a problem hiding this comment.
Not really your problem but this and the last couple of changes have moved this function from one in which all of the possible commands were handled in the if/else block below into one where some are handled above with early returns and some are handled below with early returns.
If we find ourselves modifying this again it might be nice to clean up.
Previously, the private `stream_replication.job_liveness_timeout` cluster setting determined how long the producer job would stay alive without a hearbeat from the consumer job. This patch removes this setting, and adds new sql syntax so the user can set this timeout on for all producer jobs on the given tenant. Epic: CRDB-34233 Release note (sql change): This patch adds the new EXPIRATION WINDOW option set through `ALTER TENANT appTenant SET REPLICATION EXPIRATION WINDOW ='100ms'`, which allows the user to override the default producer job expiration window of 24 hours. The producer job expiration window determines how long the producer job stays alive without a heartbeat from the consumer job.
msbutler
force-pushed
the
butler-rebased-alter-tenant-producer
branch
from
c411be4
to
de0265a
Compare
|
TFTR! bors r=stevendanna |
|
Build succeeded: |
Previously, the private
stream_replication.job_liveness_timeoutclustersetting determined how long the producer job would stay alive without a
hearbeat from the consumer job. This patch removes this setting, and adds new
sql syntax so the user can set this timeout on for all producer jobs on the
given tenant.
Epic: CRDB-34233
Release note (sql change): This patch adds the new EXPIRATION WINDOW option set
through
ALTER TENANT appTenant SET REPLICATION EXPIRATION WINDOW ='100ms',which allows the user to override the default producer job expiration window of
24 hours. The producer job expiration window determines how long the producer
job stays alive without a heartbeat from the consumer job.