sql: add timeout for PCR reader catalog lease acquisition #143669
Conversation
fqazi
requested review from
angles-n-daemons,
arjunmahishi,
aa-joshi and
Abhinav1299
and removed request for
a team
|
This change is |
fqazi
force-pushed
the
fixDebugZipAvailbilityBug
branch
3 times, most recently
from
ae0c904 to
0f3354b
Compare
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @aa-joshi, @Abhinav1299, @angles-n-daemons, and @arjunmahishi)
pkg/sql/conn_executor.go line 3837 at r1 (raw file):
// we are connecting to a PCR reader catalog, if this has not been attempted // before. func (ex *connExecutor) maybeInitPCRReaderCatalog(ctx context.Context) {
is this safe to call concurrently?
i'm wondering if the init needs to happen inside of a sync.Once, so that way if there are concurrent calls that all try to initialize it, they will block until the init is done.
fqazi
force-pushed
the
fixDebugZipAvailbilityBug
branch
from
0f3354b to
f1c965a
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/conn_executor.go line 3837 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
is this safe to call concurrently?
i'm wondering if the init needs to happen inside of a
sync.Once, so that way if there are concurrent calls that all try to initialize it, they will block until the init is done.
Done.
Good point changed this to a sync.Once
There was a problem hiding this comment.
Reviewable status:
pkg/sql/conn_executor.go line 3857 at r2 (raw file):
// unless there is some availability issue. const initPCRReaderCatalogTimeout = 30 * time.Second err := timeutil.RunWithTimeout(ctx, "detect-pcr-reader-catalog", initPCRReaderCatalogTimeout,
one more thing i wanted to ask: what if we keep the initialization code in newConnExecutor, but add the 30 second timeout there. does that resolve the test issue as well?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/conn_executor.go line 3857 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
one more thing i wanted to ask: what if we keep the initialization code in
newConnExecutor, but add the 30 second timeout there. does that resolve the test issue as well?
Yeah that also resolves the issue, I'll move it back there
There was a problem hiding this comment.
Reviewable status:
pkg/sql/conn_executor.go line 3857 at r2 (raw file):
Previously, fqazi (Faizan Qazi) wrote…
Yeah that also resolves the issue, I'll move it back there
thanks, i'd be more comfortable with that since lazy initialization usually leads to added complexity
fqazi
force-pushed
the
fixDebugZipAvailbilityBug
branch
from
f1c965a to
8e4bb15
Compare
fqazi
changed the title
There was a problem hiding this comment.
Reviewable status:
pkg/sql/conn_executor.go line 3857 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
thanks, i'd be more comfortable with that since lazy initialization usually leads to added complexity
Done.
I also reduced the timeout to 10 seconds.
pkg/sql/conn_executor.go
Outdated
| @@ -3852,6 +3843,34 @@ func (ex *connExecutor) initEvalCtx(ctx context.Context, evalCtx *extendedEvalCo | |||
| evalCtx.copyFromExecCfg(ex.server.cfg) | |||
| } | |||
|
|
|||
| // maybeInitPCRReaderCatalog leases the system database to determine if | |||
There was a problem hiding this comment.
nit: the comment should say initPCRReaderCatalog, not maybeInitPCRReaderCatalog
Previously, the logic to determine if a PCR reader catalog was in use could become stuck if an availability issue occurred with the leasing subsystem. This was because we could end up waiting indefinitely for the lease in failure scenarios like TestUnavailableZipDir, and the statement_timeout is not active this early. To address this, this patch adds a 10-second timeout for obtaining a lease on the system database when detecting PCR reader catalogs. Fixes: cockroachdb#141565 Release note: None
fqazi
force-pushed
the
fixDebugZipAvailbilityBug
branch
from
8e4bb15 to
59f3557
Compare
|
@rafiss TFTR! bors r+ |
Previously, the logic to determine if a PCR reader catalog was in use could become stuck if an availability issue occurred with the leasing subsystem. This was because we could end up waiting indefinitely for the lease in failure scenarios like TestUnavailableZipDir, and the statement_timeout is not active this early. To address this, this patch adds a 30-second timeout for obtaining a lease on the system database when detecting PCR reader catalogs.
Fixes: #141565
Release note: None