From 08362cfa2b36847c9ea1049c157c7e649a827efa Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Fri, 23 Oct 2026 18:38:22 +0000 Subject: [PATCH 1/7] test(bigtable): fix flaky backup system tests --- handwritten/bigtable/system-test/bigtable.ts | 362 +++++++++++-------- 1 file changed, 221 insertions(+), 141 deletions(-) diff --git a/handwritten/bigtable/system-test/bigtable.ts b/handwritten/bigtable/system-test/bigtable.ts index 1550e8e1c655..0d6ce7a70997 100644 --- a/handwritten/bigtable/system-test/bigtable.ts +++ b/handwritten/bigtable/system-test/bigtable.ts @@ -1385,10 +1385,12 @@ describe('Bigtable', () => { ); before(async () => { - const [backup, op] = await CLUSTER.createBackup(backupIdFromCluster, { - table: TABLE, - expireTime, - }); + const [backup, op] = await withBackupQuotaRetry(() => + CLUSTER.createBackup(backupIdFromCluster, { + table: TABLE, + expireTime, + }), + ); BACKUP = backup; await op.promise(); backupNameFromCluster = replaceProjectIdToken( @@ -1410,9 +1412,11 @@ describe('Bigtable', () => { }); it('should create backup of a table (from table)', async () => { - const [backup, op] = await TABLE.createBackup(backupIdFromTable, { - expireTime, - }); + const [backup, op] = await withBackupQuotaRetry(() => + TABLE.createBackup(backupIdFromTable, { + expireTime, + }), + ); await op.promise(); await backup.getMetadata(); @@ -1504,9 +1508,11 @@ describe('Bigtable', () => { it('should update a backup (cluster)', async () => { const backup = CLUSTER.backup(backupIdFromCluster); - const [metadata] = await backup.setMetadata({ - expireTime: updateExpireTime, - }); + const [metadata] = await withBackupQuotaRetry(() => + backup.setMetadata({ + expireTime: updateExpireTime, + }), + ); assert.strictEqual(metadata.name, backupNameFromCluster); assert.deepStrictEqual(backup.expireDate, updateExpireTime); @@ -1570,7 +1576,9 @@ describe('Bigtable', () => { const [backupsBeforeCopy] = await instance.getBackups(); const backupIdsBeforeCopy = backupsBeforeCopy.map(backup => backup.id); // Copy the backup - const [newBackup, operation] = await backup.copy(config); + const [newBackup, operation] = await withBackupQuotaRetry(() => + backup.copy(config), + ); try { assert.strictEqual(config.id, newBackup.id); await operation.promise(); @@ -1604,7 +1612,9 @@ describe('Bigtable', () => { assert.strictEqual(fetchedNewBackup.name, backupPath); // Delete the copied backup } finally { - await config.cluster.backup(newBackup.id).delete(); + await withBackupQuotaRetry(() => + config.cluster.backup(newBackup.id).delete(), + ); } } @@ -1613,9 +1623,11 @@ describe('Bigtable', () => { sourceTestExpireTime: BackupTimestamp, copyTestExpireTime: BackupTimestamp, ) { - const [backup, op] = await TABLE.createBackup(generateId('backup'), { - expireTime: sourceTestExpireTime, - }); + const [backup, op] = await withBackupQuotaRetry(() => + TABLE.createBackup(generateId('backup'), { + expireTime: sourceTestExpireTime, + }), + ); try { { await op.promise(); @@ -1633,7 +1645,7 @@ describe('Bigtable', () => { INSTANCE, ); } finally { - await backup.delete(); + await withBackupQuotaRetry(() => backup.delete()); } } it('should copy to the same cluster with precise date expiry times', async () => { @@ -1655,9 +1667,11 @@ describe('Bigtable', () => { }); }); it('should create backup of a table and copy it on another cluster of another instance', async () => { - const [backup, op] = await TABLE.createBackup(generateId('backup'), { - expireTime: sourceExpireTime, - }); + const [backup, op] = await withBackupQuotaRetry(() => + TABLE.createBackup(generateId('backup'), { + expireTime: sourceExpireTime, + }), + ); try { { await op.promise(); @@ -1668,45 +1682,48 @@ describe('Bigtable', () => { // Create another instance const instance = bigtable.instance(generateId('instance')); const destinationClusterId = generateId('cluster'); - { - // Create production instance with given options - const instanceOptions: InstanceOptions = { - clusters: [ - { - id: destinationClusterId, - nodes: 1, - location: 'us-central2-d', - storage: 'ssd', - }, - ], - labels: { - 'prod-label': 'prod-label', - time_created: String(Date.now()), + // Create production instance with given options + const instanceOptions: InstanceOptions = { + clusters: [ + { + id: destinationClusterId, + nodes: 1, + location: 'us-central2-d', + storage: 'ssd', }, - type: 'production', - }; - const [, operation] = await instance.create(instanceOptions); + ], + labels: { + 'prod-label': 'prod-label', + time_created: String(Date.now()), + }, + type: 'production', + }; + const [, operation] = await instance.create(instanceOptions); + try { await operation.promise(); + // Create the copy and test the copied backup + await testCopyBackup( + backup, + { + cluster: new Cluster(instance, destinationClusterId), + id: generateId('backup'), + expireTime: copyExpireTime, + }, + instance, + ); + } finally { + await instance.delete(); } - // Create the copy and test the copied backup - await testCopyBackup( - backup, - { - cluster: new Cluster(instance, destinationClusterId), - id: generateId('backup'), - expireTime: copyExpireTime, - }, - instance, - ); - await instance.delete(); } finally { - await backup.delete(); + await withBackupQuotaRetry(() => backup.delete()); } }); it('should create backup of a table and copy it on another cluster of the same instance', async () => { - const [backup, op] = await TABLE.createBackup(generateId('backup'), { - expireTime: sourceExpireTime, - }); + const [backup, op] = await withBackupQuotaRetry(() => + TABLE.createBackup(generateId('backup'), { + expireTime: sourceExpireTime, + }), + ); try { { await op.promise(); @@ -1715,34 +1732,37 @@ describe('Bigtable', () => { assert.deepStrictEqual(backup.expireDate, sourceExpireTime); } const destinationClusterId = generateId('cluster'); - { - // Create destination cluster with given options - const [, operation] = await INSTANCE.cluster( - destinationClusterId, - ).create({ - location: 'us-central2-b', - nodes: 1, - }); + const destinationCluster = INSTANCE.cluster(destinationClusterId); + // Create destination cluster with given options + const [, operation] = await destinationCluster.create({ + location: 'us-central2-b', + nodes: 1, + }); + try { await operation.promise(); + // Create the copy and test the copied backup + await testCopyBackup( + backup, + { + cluster: new Cluster(INSTANCE, destinationClusterId), + id: generateId('backup'), + expireTime: copyExpireTime, + }, + INSTANCE, + ); + } finally { + await destinationCluster.delete(); } - // Create the copy and test the copied backup - await testCopyBackup( - backup, - { - cluster: new Cluster(INSTANCE, destinationClusterId), - id: generateId('backup'), - expireTime: copyExpireTime, - }, - INSTANCE, - ); } finally { - await backup.delete(); + await withBackupQuotaRetry(() => backup.delete()); } }); it('should create backup of a table and copy it on another project', async () => { - const [backup, op] = await TABLE.createBackup(generateId('backup'), { - expireTime: sourceExpireTime, - }); + const [backup, op] = await withBackupQuotaRetry(() => + TABLE.createBackup(generateId('backup'), { + expireTime: sourceExpireTime, + }), + ); try { { await op.promise(); @@ -1760,42 +1780,47 @@ describe('Bigtable', () => { generateId('instance'), ); const destinationClusterId = generateId('cluster'); - { - // Create production instance with given options - const instanceOptions: InstanceOptions = { - clusters: [ - { - id: destinationClusterId, - nodes: 1, - location: 'us-central2-d', - storage: 'ssd', - }, - ], - labels: {'prod-label': 'prod-label'}, - type: 'production', - }; - const [, operation] = await secondInstance.create(instanceOptions); + // Create production instance with given options + const instanceOptions: InstanceOptions = { + clusters: [ + { + id: destinationClusterId, + nodes: 1, + location: 'us-central2-d', + storage: 'ssd', + }, + ], + labels: {'prod-label': 'prod-label'}, + type: 'production', + }; + const [, operation] = await secondInstance.create(instanceOptions); + try { await operation.promise(); + // Create the copy and test the copied backup + await testCopyBackup( + backup, + { + cluster: new Cluster(secondInstance, destinationClusterId), + id: generateId('backup'), + expireTime: copyExpireTime, + }, + secondInstance, + ); + } finally { + await secondInstance.delete(); } - // Create the copy and test the copied backup - await testCopyBackup( - backup, - { - cluster: new Cluster(secondInstance, destinationClusterId), - id: generateId('backup'), - expireTime: copyExpireTime, - }, - secondInstance, - ); - await secondInstance.delete(); } finally { - await backup.delete(); + await withBackupQuotaRetry(() => backup.delete()); } }); it('should restore a copied backup', async () => { const backupId = generateId('backup'); - const table = INSTANCE.table('old-table'); - { + const table = INSTANCE.table(generateId('table')); + const restoredTableId = generateId('table'); + let backup: Backup | undefined; + let newBackup: Backup | undefined; + let newTable: Table | undefined; + try { // Create a table and insert data into it. await table.create(); await table.createFamily('follows'); @@ -1809,15 +1834,15 @@ describe('Bigtable', () => { }, }, ]); - } - // Create the backup - const [backup, createBackupOperation] = await table.createBackup( - backupId, - { - expireTime: sourceExpireTime, - }, - ); - try { + await table.waitForReplication(); + // Create the backup + const [createdBackup, createBackupOperation] = + await withBackupQuotaRetry(() => + table.createBackup(backupId, { + expireTime: sourceExpireTime, + }), + ); + backup = createdBackup; await createBackupOperation.promise(); // Copy the backup const config = { @@ -1825,16 +1850,45 @@ describe('Bigtable', () => { id: generateId('backup'), expireTime: copyExpireTime, }; - const [newBackup, copyOperation] = await backup.copy(config); + const [copiedBackup, copyOperation] = await withBackupQuotaRetry(() => + backup!.copy(config), + ); + newBackup = copiedBackup; await copyOperation.promise(); // Restore a table from the copied backup - const [newTable, restoreOperation] = - await newBackup.restore('new-table'); + const [restoredTable, restoreOperation] = + await newBackup.restore(restoredTableId); + newTable = restoredTable; await restoreOperation.promise(); const rows = await newTable.getRows(); assert.deepStrictEqual(rows[0][0].id, 'some-data-to-copy-key'); } finally { - await backup.delete(); + if (newBackup) { + try { + await withBackupQuotaRetry(() => newBackup!.delete()); + } catch { + // Ignore cleanup errors if the copied backup was not created. + } + } + if (backup) { + try { + await withBackupQuotaRetry(() => backup!.delete()); + } catch { + // Ignore cleanup errors if the source backup was not created. + } + } + if (newTable) { + try { + await newTable.delete(); + } catch { + // Ignore cleanup errors if the restored table was not created. + } + } + try { + await table.delete(); + } catch { + // Ignore cleanup errors if the source table was not created. + } } }); }); @@ -1848,9 +1902,6 @@ describe('Bigtable', () => { let backupSSD: IBackup, backupHDD: IBackup; - const targetTableIdSSD = generateId('ttbl'), - targetTableIdHDD = generateId('ttbl'); - // We just test from table here. const backupIdFromTableSSD = generateId('backup'), backupIdFromTableHDD = generateId('backup'); @@ -1871,31 +1922,39 @@ describe('Bigtable', () => { }); before(async () => { - const [backupOpSSD] = await tableAdmin.createBackup({ - parent: replaceProjectIdToken( - INSTANCE.cluster(CLUSTER_ID).name, - bigtable.projectId, - ), - backupId: backupIdFromTableSSD, - backup: { - expireTime, - sourceTable: replaceProjectIdToken(TABLE.name, bigtable.projectId), - }, - }); - const [backupOpHDD] = await tableAdmin.createBackup({ - parent: replaceProjectIdToken( - INSTANCE_HDD.cluster(CLUSTER_ID_HDD).name, - bigtable.projectId, - ), - backupId: backupIdFromTableHDD, - backup: { - expireTime, - sourceTable: replaceProjectIdToken( - TABLE_HDD.name, + // Sleep here for just over a minute before the "before all" hook creates + // backups so that requests from the preceding backup tests do not trigger + // per-minute 'Backup metadata write requests' quota exhaustion. + await new Promise(r => setTimeout(r, 60001)); + const [backupOpSSD] = await withBackupQuotaRetry(() => + tableAdmin.createBackup({ + parent: replaceProjectIdToken( + INSTANCE.cluster(CLUSTER_ID).name, bigtable.projectId, ), - }, - }); + backupId: backupIdFromTableSSD, + backup: { + expireTime, + sourceTable: replaceProjectIdToken(TABLE.name, bigtable.projectId), + }, + }), + ); + const [backupOpHDD] = await withBackupQuotaRetry(() => + tableAdmin.createBackup({ + parent: replaceProjectIdToken( + INSTANCE_HDD.cluster(CLUSTER_ID_HDD).name, + bigtable.projectId, + ), + backupId: backupIdFromTableHDD, + backup: { + expireTime, + sourceTable: replaceProjectIdToken( + TABLE_HDD.name, + bigtable.projectId, + ), + }, + }), + ); const [resSSD, resHDD] = await Promise.all([ backupOpSSD.promise(), backupOpHDD.promise(), @@ -1932,6 +1991,7 @@ describe('Bigtable', () => { it('should not optimize when restoring from the same backing type', async () => { // HDD -> HDD and SSD -> SSD should both not trigger an optimize pass. + const targetTableIdHDD = generateId('ttbl'); const [restoreOp] = await tableAdmin.restoreTable({ parent: replaceProjectIdToken(INSTANCE_HDD.name, bigtable.projectId), tableId: targetTableIdHDD, @@ -1943,6 +2003,7 @@ describe('Bigtable', () => { it('should optimize when restoring from a different backing type', async () => { // HDD -> SSD should trigger an optimize. + const targetTableIdSSD = generateId('ttbl'); const [restoreOp] = await tableAdmin.restoreTable({ parent: replaceProjectIdToken(INSTANCE.name, bigtable.projectId), tableId: targetTableIdSSD, @@ -2624,3 +2685,22 @@ function createInstanceConfig( }, }; } + +async function withBackupQuotaRetry( + fn: () => Promise, + retries = 3, +): Promise { + for (let attempt = 0; ; attempt++) { + try { + return await fn(); + } catch (err) { + const isQuotaError = + (err as ServiceError)?.code === 8 || + (err as Error)?.message?.includes('RESOURCE_EXHAUSTED'); + if (!isQuotaError || attempt >= retries) { + throw err; + } + await new Promise(resolve => setTimeout(resolve, 60001)); + } + } +} From d57cbd16b4de7ea5fb387e65d96e9ae59bca9720 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Fri, 23 Oct 2026 18:49:44 +0000 Subject: [PATCH 2/7] [Style Maintenance] align changes with CONTRIBUTING.md and gts coding style guidelines --- handwritten/bigtable/system-test/bigtable.ts | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/handwritten/bigtable/system-test/bigtable.ts b/handwritten/bigtable/system-test/bigtable.ts index 0d6ce7a70997..9cb56dcd33f0 100644 --- a/handwritten/bigtable/system-test/bigtable.ts +++ b/handwritten/bigtable/system-test/bigtable.ts @@ -44,6 +44,8 @@ import google = protos.google; type IBackup = google.bigtable.admin.v2.IBackup; +const BACKUP_QUOTA_COOLDOWN_MS = 60001; + describe('Bigtable', () => { const bigtable = new Bigtable(); const INSTANCE = bigtable.instance(generateId('instance')); @@ -1560,7 +1562,7 @@ describe('Bigtable', () => { // Sleep here for just over a minute so that the system tests don't // experience quota issues due to too many requests per minute. await new Promise(resolve => { - setTimeout(resolve, 60001); + setTimeout(resolve, BACKUP_QUOTA_COOLDOWN_MS); }); }); /* @@ -1918,14 +1920,18 @@ describe('Bigtable', () => { beforeEach(async () => { // This is not ideal, but we are running into quota issues for admin API access. - await new Promise(r => setTimeout(r, 60 * 1000)); + await new Promise(resolve => + setTimeout(resolve, BACKUP_QUOTA_COOLDOWN_MS), + ); }); before(async () => { // Sleep here for just over a minute before the "before all" hook creates // backups so that requests from the preceding backup tests do not trigger // per-minute 'Backup metadata write requests' quota exhaustion. - await new Promise(r => setTimeout(r, 60001)); + await new Promise(resolve => + setTimeout(resolve, BACKUP_QUOTA_COOLDOWN_MS), + ); const [backupOpSSD] = await withBackupQuotaRetry(() => tableAdmin.createBackup({ parent: replaceProjectIdToken( @@ -2686,6 +2692,10 @@ function createInstanceConfig( }; } +/** + * Executes a backup metadata write operation and retries after a 1-minute + * cooldown when a `RESOURCE_EXHAUSTED` (gRPC code 8) quota error is returned. + */ async function withBackupQuotaRetry( fn: () => Promise, retries = 3, @@ -2700,7 +2710,9 @@ async function withBackupQuotaRetry( if (!isQuotaError || attempt >= retries) { throw err; } - await new Promise(resolve => setTimeout(resolve, 60001)); + await new Promise(resolve => + setTimeout(resolve, BACKUP_QUOTA_COOLDOWN_MS), + ); } } } From c6ddbd2a031457503f30d61ca06005c50661c9f9 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Fri, 23 Oct 2026 19:00:16 +0000 Subject: [PATCH 3/7] [Independent review follow-ups] address independent Gemini review comments (round 1) --- handwritten/bigtable/system-test/bigtable.ts | 130 +++++++++++++------ 1 file changed, 91 insertions(+), 39 deletions(-) diff --git a/handwritten/bigtable/system-test/bigtable.ts b/handwritten/bigtable/system-test/bigtable.ts index 9cb56dcd33f0..51edf5cab65a 100644 --- a/handwritten/bigtable/system-test/bigtable.ts +++ b/handwritten/bigtable/system-test/bigtable.ts @@ -1372,10 +1372,6 @@ describe('Bigtable', () => { // the real functionality - it is just a way to keep things organized! const backupIdFromCluster = generateId('backup'); let backupNameFromCluster: string; - const restoreTableIdFromCluster = generateId('table'); - - const backupIdFromTable = generateId('backup'); - let backupNameFromTable: string; // The minimum backup expiry time is 6 hours. The times here each have a 2 // hour padding to tolerate latency and clock drift. Also, while the time @@ -1399,10 +1395,6 @@ describe('Bigtable', () => { `${CLUSTER.name}/backups/${backupIdFromCluster}`, bigtable.projectId, ); - backupNameFromTable = replaceProjectIdToken( - `${CLUSTER.name}/backups/${backupIdFromTable}`, - bigtable.projectId, - ); }); it('should create backup of a table (from cluster)', async () => { @@ -1414,17 +1406,30 @@ describe('Bigtable', () => { }); it('should create backup of a table (from table)', async () => { + const backupIdFromTable = generateId('backup'); + const backupNameFromTable = replaceProjectIdToken( + `${CLUSTER.name}/backups/${backupIdFromTable}`, + bigtable.projectId, + ); const [backup, op] = await withBackupQuotaRetry(() => TABLE.createBackup(backupIdFromTable, { expireTime, }), ); - await op.promise(); - await backup.getMetadata(); + try { + await op.promise(); + await backup.getMetadata(); - assert.strictEqual(backup.metadata!.name, backupNameFromTable); + assert.strictEqual(backup.metadata!.name, backupNameFromTable); - assert.deepStrictEqual(backup.expireDate, expireTime); + assert.deepStrictEqual(backup.expireDate, expireTime); + } finally { + try { + await withBackupQuotaRetry(() => backup.delete()); + } catch { + // Ignore cleanup errors if the backup was not created. + } + } }); it('should get a specific backup (cluster)', async () => { @@ -1476,6 +1481,7 @@ describe('Bigtable', () => { }); it('should restore a backup (cluster)', async () => { + const restoreTableIdFromCluster = generateId('table'); const backup = CLUSTER.backup(backupIdFromCluster); const [table, op] = await backup.restore(restoreTableIdFromCluster); await op.promise(); @@ -1485,18 +1491,22 @@ describe('Bigtable', () => { }); it('should restore a backup to a different instance', async () => { - const [, operation] = await DIFF_INSTANCE.create( - createInstanceConfig( - generateId('d-clust'), - 'us-east1-c', - 3, - Date.now(), - ), - ); - await operation.promise(); + const [alreadyExists] = await DIFF_INSTANCE.exists(); + if (!alreadyExists) { + const [, operation] = await DIFF_INSTANCE.create( + createInstanceConfig( + generateId('d-clust'), + 'us-east1-c', + 3, + Date.now(), + ), + ); + await operation.promise(); + } const [iExists] = await DIFF_INSTANCE.exists(); assert.strictEqual(iExists, true); + const restoreTableIdFromCluster = generateId('table'); const backup = CLUSTER.backup(backupIdFromCluster); const [table, op] = await backup.restoreTo({ tableId: restoreTableIdFromCluster, @@ -1614,9 +1624,13 @@ describe('Bigtable', () => { assert.strictEqual(fetchedNewBackup.name, backupPath); // Delete the copied backup } finally { - await withBackupQuotaRetry(() => - config.cluster.backup(newBackup.id).delete(), - ); + try { + await withBackupQuotaRetry(() => + config.cluster.backup(newBackup.id).delete(), + ); + } catch { + // Ignore cleanup errors if the copied backup was not created. + } } } @@ -1647,7 +1661,11 @@ describe('Bigtable', () => { INSTANCE, ); } finally { - await withBackupQuotaRetry(() => backup.delete()); + try { + await withBackupQuotaRetry(() => backup.delete()); + } catch { + // Ignore cleanup errors if the source backup was not created. + } } } it('should copy to the same cluster with precise date expiry times', async () => { @@ -1714,10 +1732,19 @@ describe('Bigtable', () => { instance, ); } finally { - await instance.delete(); + await reapBackups(instance); + try { + await instance.delete(); + } catch { + // Ignore cleanup errors if the instance was not created. + } } } finally { - await withBackupQuotaRetry(() => backup.delete()); + try { + await withBackupQuotaRetry(() => backup.delete()); + } catch { + // Ignore cleanup errors if the source backup was not created. + } } }); it('should create backup of a table and copy it on another cluster of the same instance', async () => { @@ -1735,6 +1762,7 @@ describe('Bigtable', () => { } const destinationClusterId = generateId('cluster'); const destinationCluster = INSTANCE.cluster(destinationClusterId); + const copiedBackupId = generateId('backup'); // Create destination cluster with given options const [, operation] = await destinationCluster.create({ location: 'us-central2-b', @@ -1747,16 +1775,29 @@ describe('Bigtable', () => { backup, { cluster: new Cluster(INSTANCE, destinationClusterId), - id: generateId('backup'), + id: copiedBackupId, expireTime: copyExpireTime, }, INSTANCE, ); } finally { - await destinationCluster.delete(); + try { + await destinationCluster.backup(copiedBackupId).delete(); + } catch { + // Ignore cleanup errors if the copied backup was already deleted. + } + try { + await destinationCluster.delete(); + } catch { + // Ignore cleanup errors if the destination cluster was not created. + } } } finally { - await withBackupQuotaRetry(() => backup.delete()); + try { + await withBackupQuotaRetry(() => backup.delete()); + } catch { + // Ignore cleanup errors if the source backup was not created. + } } }); it('should create backup of a table and copy it on another project', async () => { @@ -1809,10 +1850,19 @@ describe('Bigtable', () => { secondInstance, ); } finally { - await secondInstance.delete(); + await reapBackups(secondInstance); + try { + await secondInstance.delete(); + } catch { + // Ignore cleanup errors if the second instance was not created. + } } } finally { - await withBackupQuotaRetry(() => backup.delete()); + try { + await withBackupQuotaRetry(() => backup.delete()); + } catch { + // Ignore cleanup errors if the source backup was not created. + } } }); it('should restore a copied backup', async () => { @@ -1848,33 +1898,35 @@ describe('Bigtable', () => { await createBackupOperation.promise(); // Copy the backup const config = { - cluster: backup.cluster, + cluster: createdBackup.cluster, id: generateId('backup'), expireTime: copyExpireTime, }; const [copiedBackup, copyOperation] = await withBackupQuotaRetry(() => - backup!.copy(config), + createdBackup.copy(config), ); newBackup = copiedBackup; await copyOperation.promise(); // Restore a table from the copied backup const [restoredTable, restoreOperation] = - await newBackup.restore(restoredTableId); + await copiedBackup.restore(restoredTableId); newTable = restoredTable; await restoreOperation.promise(); - const rows = await newTable.getRows(); + const rows = await restoredTable.getRows(); assert.deepStrictEqual(rows[0][0].id, 'some-data-to-copy-key'); } finally { if (newBackup) { + const copiedBackupToDelete = newBackup; try { - await withBackupQuotaRetry(() => newBackup!.delete()); + await withBackupQuotaRetry(() => copiedBackupToDelete.delete()); } catch { // Ignore cleanup errors if the copied backup was not created. } } if (backup) { + const sourceBackupToDelete = backup; try { - await withBackupQuotaRetry(() => backup!.delete()); + await withBackupQuotaRetry(() => sourceBackupToDelete.delete()); } catch { // Ignore cleanup errors if the source backup was not created. } @@ -2698,7 +2750,7 @@ function createInstanceConfig( */ async function withBackupQuotaRetry( fn: () => Promise, - retries = 3, + retries = 2, ): Promise { for (let attempt = 0; ; attempt++) { try { From 1cf29f827450f3e52616093b50014dcfa86785f3 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Fri, 23 Oct 2026 19:06:42 +0000 Subject: [PATCH 4/7] [Independent review follow-ups] address independent Gemini review comments (round 2) --- handwritten/bigtable/system-test/bigtable.ts | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/handwritten/bigtable/system-test/bigtable.ts b/handwritten/bigtable/system-test/bigtable.ts index 51edf5cab65a..0fdc10694987 100644 --- a/handwritten/bigtable/system-test/bigtable.ts +++ b/handwritten/bigtable/system-test/bigtable.ts @@ -1762,7 +1762,6 @@ describe('Bigtable', () => { } const destinationClusterId = generateId('cluster'); const destinationCluster = INSTANCE.cluster(destinationClusterId); - const copiedBackupId = generateId('backup'); // Create destination cluster with given options const [, operation] = await destinationCluster.create({ location: 'us-central2-b', @@ -1775,17 +1774,12 @@ describe('Bigtable', () => { backup, { cluster: new Cluster(INSTANCE, destinationClusterId), - id: copiedBackupId, + id: generateId('backup'), expireTime: copyExpireTime, }, INSTANCE, ); } finally { - try { - await destinationCluster.backup(copiedBackupId).delete(); - } catch { - // Ignore cleanup errors if the copied backup was already deleted. - } try { await destinationCluster.delete(); } catch { From c05842418b06471ab0e0e466cb047c466b3dfe8e Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Mon, 26 Oct 2026 18:11:21 +0000 Subject: [PATCH 5/7] [Address CI errors] check restored table existence on DIFF_INSTANCE in backup system test --- handwritten/bigtable/system-test/bigtable.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/handwritten/bigtable/system-test/bigtable.ts b/handwritten/bigtable/system-test/bigtable.ts index 0fdc10694987..fc1b32f89a9d 100644 --- a/handwritten/bigtable/system-test/bigtable.ts +++ b/handwritten/bigtable/system-test/bigtable.ts @@ -1513,7 +1513,9 @@ describe('Bigtable', () => { instance: DIFF_INSTANCE, }); await op.promise(); - const [tExists] = await table.exists(); + const [tExists] = await DIFF_INSTANCE.table( + restoreTableIdFromCluster, + ).exists(); assert.strictEqual(tExists, true); assert.strictEqual(table.id, restoreTableIdFromCluster); }); @@ -2751,7 +2753,7 @@ async function withBackupQuotaRetry( return await fn(); } catch (err) { const isQuotaError = - (err as ServiceError)?.code === 8 || + (err as {code?: number})?.code === 8 || (err as Error)?.message?.includes('RESOURCE_EXHAUSTED'); if (!isQuotaError || attempt >= retries) { throw err; From 04d269228ad0cc7cca8d3121468b8d9a65b97dd0 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Mon, 26 Oct 2026 20:34:39 +0000 Subject: [PATCH 6/7] [Address CI errors] fix circular require in mutateInternal and client-side metrics export interval --- .../src/client-side-metrics/exporter.ts | 8 ++- .../bigtable/src/utils/mutateInternal.ts | 9 +-- .../client-side-metrics-all-methods.ts | 70 ++++++++++--------- .../system-test/client-side-metrics.ts | 70 ++++++++++--------- 4 files changed, 85 insertions(+), 72 deletions(-) diff --git a/handwritten/bigtable/src/client-side-metrics/exporter.ts b/handwritten/bigtable/src/client-side-metrics/exporter.ts index bfacf014951e..9f8a3bca813f 100644 --- a/handwritten/bigtable/src/client-side-metrics/exporter.ts +++ b/handwritten/bigtable/src/client-side-metrics/exporter.ts @@ -53,9 +53,13 @@ function getInterval( const startSec = dataPoint.startTime[0]; const startNanos = dataPoint.startTime[1]; - if (startSec > endSec || (startSec === endSec && startNanos > endNanos)) { + if (startSec > endSec || (startSec === endSec && startNanos >= endNanos)) { endSec = startSec; - endNanos = startNanos; + endNanos = startNanos + 1_000_000; + if (endNanos >= 1_000_000_000) { + endSec += 1; + endNanos -= 1_000_000_000; + } } return { diff --git a/handwritten/bigtable/src/utils/mutateInternal.ts b/handwritten/bigtable/src/utils/mutateInternal.ts index 1e084b92a791..146f5527dbad 100644 --- a/handwritten/bigtable/src/utils/mutateInternal.ts +++ b/handwritten/bigtable/src/utils/mutateInternal.ts @@ -12,16 +12,13 @@ // See the License for the specific language governing permissions and // limitations under the License. -import { - Entry, - MutateCallback, - MutateOptions, - PartialFailureError, -} from '../table'; +import {Entry, MutateOptions} from '../table'; import {OperationMetricsCollector} from '../client-side-metrics/operation-metrics-collector'; import { DEFAULT_BACKOFF_SETTINGS, getNextDelay, + MutateCallback, + PartialFailureError, populateAttemptHeader, RETRYABLE_STATUS_CODES, TabularApiSurface, diff --git a/handwritten/bigtable/system-test/client-side-metrics-all-methods.ts b/handwritten/bigtable/system-test/client-side-metrics-all-methods.ts index d0d8d9ebe49b..569c236f666a 100644 --- a/handwritten/bigtable/system-test/client-side-metrics-all-methods.ts +++ b/handwritten/bigtable/system-test/client-side-metrics-all-methods.ts @@ -357,7 +357,7 @@ async function checkForPublishedMetrics(projectId: string) { seriesLength = series.length; break; } - await new Promise(r => setTimeout(r, 2000)); + await new Promise(resolve => setTimeout(resolve, 2000)); } assert(seriesLength > 0); } @@ -482,10 +482,10 @@ describe('Bigtable/ClientSideMetricsAllMethods', () => { metrics: ResourceMetrics, resultCallback: (result: ExportResult) => void, ): Promise { - try { - await super.export(metrics, (result: ExportResult) => { - if (!exported) { - exported = true; + if (!exported) { + exported = true; + try { + await super.export(metrics, (result: ExportResult) => { try { clearTimeout(timeout); // The test passes when the code is 0 because that means the @@ -493,6 +493,7 @@ describe('Bigtable/ClientSideMetricsAllMethods', () => { assert.strictEqual(result.code, 0); resultCallback({code: 0}); void checkForPublishedMetrics(projectId) + // eslint-disable-next-line promise/always-return .then(() => { done(); }) @@ -503,12 +504,12 @@ describe('Bigtable/ClientSideMetricsAllMethods', () => { // The code here isn't 0 so we report the original error to the mocha test runner. done(error); } - } else { - resultCallback({code: 0}); - } - }); - } catch (error) { - done(error); + }); + } catch (error) { + done(error); + } + } else { + resultCallback({code: 0}); } } } @@ -1025,6 +1026,7 @@ describe('Bigtable/ClientSideMetricsAllMethods', () => { done: mocha.Done, onExportSuccess?: () => void, ) { + let exported = false; class TestExporter extends CloudMonitoringExporter { constructor(options: ClientOptions) { super(options); @@ -1034,28 +1036,32 @@ describe('Bigtable/ClientSideMetricsAllMethods', () => { metrics: ResourceMetrics, resultCallback: (result: ExportResult) => void, ): Promise { - try { - await super.export(metrics, async (result: ExportResult) => { - try { - // The code is expected to be 0 because the - // result from calling export was successful. - assert.strictEqual(result.code, 0); - resultCallback({code: 0}); - if (onExportSuccess) { - onExportSuccess(); + if (!exported) { + exported = true; + try { + await super.export(metrics, async (result: ExportResult) => { + try { + // The code is expected to be 0 because the + // result from calling export was successful. + assert.strictEqual(result.code, 0); + resultCallback({code: 0}); + if (onExportSuccess) { + onExportSuccess(); + } + } catch { + // The code here isn't 0 so we report the original error to the + // mocha test runner. + // The test fails here because it means that an export was + // unsuccessful. + done(result); + resultCallback({code: 0}); } - } catch (error) { - // The code here isn't 0 so we report the original error to the - // mocha test runner. - // The test fails here because it means that an export was - // unsuccessful. - done(result); - done(error); - resultCallback({code: 0}); - } - }); - } catch (error) { - done(error); + }); + } catch (error) { + done(error); + resultCallback({code: 0}); + } + } else { resultCallback({code: 0}); } } diff --git a/handwritten/bigtable/system-test/client-side-metrics.ts b/handwritten/bigtable/system-test/client-side-metrics.ts index 7a029b37378e..35bd3f157407 100644 --- a/handwritten/bigtable/system-test/client-side-metrics.ts +++ b/handwritten/bigtable/system-test/client-side-metrics.ts @@ -334,7 +334,7 @@ async function checkForPublishedMetrics(projectId: string) { seriesLength = series.length; break; } - await new Promise(r => setTimeout(r, 2000)); + await new Promise(resolve => setTimeout(resolve, 2000)); } assert(seriesLength > 0); } @@ -459,10 +459,10 @@ describe('Bigtable/ClientSideMetrics', () => { metrics: ResourceMetrics, resultCallback: (result: ExportResult) => void, ): Promise { - try { - await super.export(metrics, (result: ExportResult) => { - if (!exported) { - exported = true; + if (!exported) { + exported = true; + try { + await super.export(metrics, (result: ExportResult) => { try { clearTimeout(timeout); // The test passes when the code is 0 because that means the @@ -470,6 +470,7 @@ describe('Bigtable/ClientSideMetrics', () => { assert.strictEqual(result.code, 0); resultCallback({code: 0}); void checkForPublishedMetrics(projectId) + // eslint-disable-next-line promise/always-return .then(() => { done(); }) @@ -480,12 +481,12 @@ describe('Bigtable/ClientSideMetrics', () => { // The code here isn't 0 so we report the original error to the mocha test runner. done(error); } - } else { - resultCallback({code: 0}); - } - }); - } catch (error) { - done(error); + }); + } catch (error) { + done(error); + } + } else { + resultCallback({code: 0}); } } } @@ -582,6 +583,7 @@ describe('Bigtable/ClientSideMetrics', () => { done: mocha.Done, onExportSuccess?: () => void, ) { + let exported = false; class TestExporter extends CloudMonitoringExporter { constructor(options: ClientOptions) { super(options); @@ -591,28 +593,32 @@ describe('Bigtable/ClientSideMetrics', () => { metrics: ResourceMetrics, resultCallback: (result: ExportResult) => void, ): Promise { - try { - await super.export(metrics, async (result: ExportResult) => { - try { - // The code is expected to be 0 because the - // result from calling export was successful. - assert.strictEqual(result.code, 0); - resultCallback({code: 0}); - if (onExportSuccess) { - onExportSuccess(); + if (!exported) { + exported = true; + try { + await super.export(metrics, async (result: ExportResult) => { + try { + // The code is expected to be 0 because the + // result from calling export was successful. + assert.strictEqual(result.code, 0); + resultCallback({code: 0}); + if (onExportSuccess) { + onExportSuccess(); + } + } catch { + // The code here isn't 0 so we report the original error to the + // mocha test runner. + // The test fails here because it means that an export was + // unsuccessful. + done(result); + resultCallback({code: 0}); } - } catch (error) { - // The code here isn't 0 so we report the original error to the - // mocha test runner. - // The test fails here because it means that an export was - // unsuccessful. - done(result); - done(error); - resultCallback({code: 0}); - } - }); - } catch (error) { - done(error); + }); + } catch (error) { + done(error); + resultCallback({code: 0}); + } + } else { resultCallback({code: 0}); } } From fd560376eca46609192b8fd4de8457fe1664e3cf Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Tue, 27 Oct 2026 21:51:24 +0000 Subject: [PATCH 7/7] [Address CI errors] restore waitForReplication fake timer spies via sandbox --- handwritten/bigtable/test/table.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/handwritten/bigtable/test/table.ts b/handwritten/bigtable/test/table.ts index c10e89145d4e..19f1abd46565 100644 --- a/handwritten/bigtable/test/table.ts +++ b/handwritten/bigtable/test/table.ts @@ -2072,12 +2072,12 @@ describe('Bigtable/Table', () => { let responses: any[] = []; beforeEach(() => { - clock = sinon.useFakeTimers({ + clock = sandbox.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'], }); - setTimeoutSpy = sinon.spy(global, 'setTimeout'); - clearTimeoutSpy = sinon.spy(global, 'clearTimeout'); - checkConsistencySpy = sinon.spy(table, 'checkConsistency'); + setTimeoutSpy = sandbox.spy(global, 'setTimeout'); + clearTimeoutSpy = sandbox.spy(global, 'clearTimeout'); + checkConsistencySpy = sandbox.spy(table, 'checkConsistency'); table.bigtable.request = (config: {}, callback: Function) => { responses.shift()(config, callback); @@ -2085,7 +2085,7 @@ describe('Bigtable/Table', () => { }); afterEach(() => { - clock.restore(); + sandbox.restore(); }); it('should return true if token is consistent', done => {