[IND-552] add roundtable task to take fast sync Postgres snapshots every 4 hours #912
Conversation
WalkthroughThe changes involve introducing a new feature for creating and managing fast sync snapshots of a database. A constant for a four-hour interval has been added, along with configuration options for loop intervals and snapshot identifiers. The environment configuration now supports separate S3 bucket ARNs for different snapshot types. The test file ensures the new snapshot task behaves correctly under various conditions. The snapshot task itself includes logic to determine when a new snapshot is needed based on the time elapsed since the last one. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit's AI:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (9)
- indexer/packages/base/src/constants.ts (1 hunks)
- indexer/services/roundtable/.env.test (1 hunks)
- indexer/services/roundtable/tests/tasks/take-fast-sync-snapshot.test.ts (1 hunks)
- indexer/services/roundtable/src/config.ts (4 hunks)
- indexer/services/roundtable/src/helpers/aws.ts (4 hunks)
- indexer/services/roundtable/src/helpers/sql.ts (2 hunks)
- indexer/services/roundtable/src/index.ts (2 hunks)
- indexer/services/roundtable/src/tasks/take-fast-sync-snapshot.ts (1 hunks)
- indexer/services/roundtable/src/tasks/update-research-environment.ts (3 hunks)
Additional comments: 20
indexer/services/roundtable/.env.test (1)
- 9-10: The changes to the environment variables are consistent with the PR objectives. Ensure that all references to these variables are updated across the codebase.
Verification successful
The search results confirm that the environment variables
RESEARCH_SNAPSHOT_S3_BUCKET_ARNandFAST_SYNC_SNAPSHOT_S3_BUCKET_ARNare being used in theconfig.tsandaws.tsfiles within theindexer/services/roundtabledirectory. There are no remaining references to the oldS3_BUCKET_ARNvariable, which suggests that the necessary updates to accommodate the new environment variables have been made.
- The
RESEARCH_SNAPSHOT_S3_BUCKET_ARNis used to define a constant inconfig.tsand to derive theRESEARCH_SNAPSHOT_S3_BUCKET_NAMEinaws.ts.- The
FAST_SYNC_SNAPSHOT_S3_BUCKET_ARNis used similarly in both files.Based on the provided output, it appears that the changes to the environment variables have been correctly propagated throughout the codebase.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the renamed and new environment variables are correctly used throughout the codebase. rg --type dotenv 'S3_BUCKET_ARN' rg --type dotenv 'RESEARCH_SNAPSHOT_S3_BUCKET_ARN' rg --type dotenv 'FAST_SYNC_SNAPSHOT_S3_BUCKET_ARN'Length of output: 228
Script:
#!/bin/bash # Search for occurrences of the old and new environment variables in the codebase. rg 'S3_BUCKET_ARN' rg 'RESEARCH_SNAPSHOT_S3_BUCKET_ARN' rg 'FAST_SYNC_SNAPSHOT_S3_BUCKET_ARN'Length of output: 1071
indexer/packages/base/src/constants.ts (1)
- 15-15: The addition of the
FOUR_HOURS_IN_MILLISECONDSconstant is correct and aligns with the PR objectives.indexer/services/roundtable/src/helpers/sql.ts (2)
- 1-1: The renaming of
S3_LOCATION_PREFIXtoRESEARCH_SNAPSHOT_S3_LOCATION_PREFIXis consistent with the PR objectives. Ensure that all references to this constant are updated across the codebase.Verification successful
The search did not return any results, which is unexpected. I will perform a broader search without specifying the file type to ensure we check all files for the usage of the constants.
The search results confirm that the constant
RESEARCH_SNAPSHOT_S3_LOCATION_PREFIXis used in thesql.tsandaws.tsfiles, and there are no remaining references to the oldS3_LOCATION_PREFIX. This aligns with the PR objectives and the changes are correctly applied.
- The
RESEARCH_SNAPSHOT_S3_LOCATION_PREFIXis defined inaws.tsand used in bothaws.tsandsql.ts.- There are no occurrences of
S3_LOCATION_PREFIX, indicating that the renaming has been fully propagated.* 26-26: The usage of `RESEARCH_SNAPSHOT_S3_LOCATION_PREFIX` in the `getExternalAthenaTableCreationStatement` function is correct and aligns with the changes made to the import statement.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the renamed constant is correctly used throughout the codebase. rg --type js 'S3_LOCATION_PREFIX' rg --type js 'RESEARCH_SNAPSHOT_S3_LOCATION_PREFIX'Length of output: 85
Script:
#!/bin/bash # Broaden the search to all files for both the old and new constant names. rg 'S3_LOCATION_PREFIX' rg 'RESEARCH_SNAPSHOT_S3_LOCATION_PREFIX'Length of output: 1137
indexer/services/roundtable/__tests__/tasks/take-fast-sync-snapshot.test.ts (3)
25-32: The unit test for checking if the S3 object exists is well-structured. Ensure that the test cases cover all possible scenarios and that they are passing.
34-41: The unit test for checking if an export job is in progress is well-structured. Ensure that the test cases cover all possible scenarios and that they are passing.
43-47: The unit test for starting an export job is well-structured. Ensure that the test cases cover all possible scenarios and that they are passing.
indexer/services/roundtable/src/tasks/take-fast-sync-snapshot.ts (1)
- 17-98: The logic in
take-fast-sync-snapshot.tsfor checking the existence of an S3 object, ongoing export jobs, and starting a new export job if necessary is consistent with the PR objectives. Ensure that the task is properly integrated and tested.Verification successful
The test file
take-fast-sync-snapshot.test.tsexists in the directoryindexer/services/roundtable/__tests__/tasks/, which indicates that tests have been written for thetake-fast-sync-snapshot.tstask. This aligns with the PR objectives to ensure the functionality of the new task is tested.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the task is properly integrated and tested. npm run test -- take-fast-sync-snapshot.test.tsLength of output: 120
Script:
#!/bin/bash # Search for the test file related to take-fast-sync-snapshot.ts to confirm its existence. fd 'take-fast-sync-snapshot.test.ts'Length of output: 112
indexer/services/roundtable/src/index.ts (1)
- 103-109: The conditional block to start the
takeFastSyncSnapshotTaskloop is correctly checking the configuration flag and using the appropriate interval.indexer/services/roundtable/src/config.ts (3)
18-18: The addition of the
FOUR_HOURS_IN_MILLISECONDSconstant toconfig.tsis correct and aligns with the PR objectives.70-72: The modification of the
LOOPS_INTERVAL_MS_TAKE_FAST_SYNC_SNAPSHOTSinterval setting is correct and aligns with the PR objectives.115-116: The refined S3 bucket ARN declarations to specify research and fast sync snapshot buckets are correct and align with the PR objectives. Ensure that these new variables are used correctly throughout the codebase.
Verification successful
Let's perform a more general search to ensure that the variables are not used in an unexpected file type. I will generate a new script to search for the usage of
RESEARCH_SNAPSHOT_S3_BUCKET_ARNandFAST_SYNC_SNAPSHOT_S3_BUCKET_ARNwithout specifying the file type.
The new S3 bucket ARN variables
RESEARCH_SNAPSHOT_S3_BUCKET_ARNandFAST_SYNC_SNAPSHOT_S3_BUCKET_ARNare used in theaws.tsfile to extract S3 bucket names, which aligns with the PR objectives. No issues were found with the usage of these variables based on the provided context.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the new S3 bucket ARN variables are correctly used throughout the codebase. rg --type js 'RESEARCH_SNAPSHOT_S3_BUCKET_ARN' rg --type js 'FAST_SYNC_SNAPSHOT_S3_BUCKET_ARN'Length of output: 94
Script:
#!/bin/bash # Broaden the search for the new S3 bucket ARN variables to include all file types. rg 'RESEARCH_SNAPSHOT_S3_BUCKET_ARN' rg 'FAST_SYNC_SNAPSHOT_S3_BUCKET_ARN'Length of output: 563
indexer/services/roundtable/src/helpers/aws.ts (4)
- 19-21: The renaming of
S3_BUCKET_NAMEtoRESEARCH_SNAPSHOT_S3_BUCKET_NAMEand the addition ofFAST_SYNC_SNAPSHOT_S3_BUCKET_NAMEare consistent with the PR objectives. Ensure that all references to these constants are updated across the codebase.Verification successful
The search for the constants did not produce any results, which is unexpected. I will broaden the search to ensure we're not missing any references.
The constants
RESEARCH_SNAPSHOT_S3_BUCKET_NAMEandFAST_SYNC_SNAPSHOT_S3_BUCKET_NAMEare correctly used in the codebase, and there are no remaining references to the oldS3_BUCKET_NAMEconstant. The changes are consistent with the PR objectives.* 49-53: The modification of the `checkIfS3ObjectExists` function signature to include a `bucket` parameter is correct and aligns with the PR objectives. Ensure that all calls to this function are updated accordingly.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the renamed and new constants are correctly used throughout the codebase. rg --type js 'S3_BUCKET_NAME' rg --type js 'RESEARCH_SNAPSHOT_S3_BUCKET_NAME' rg --type js 'FAST_SYNC_SNAPSHOT_S3_BUCKET_NAME'Length of output: 126
Script:
#!/bin/bash # Broaden the search to include all files, not just JavaScript files, to ensure we find all references to the constants. rg 'S3_BUCKET_NAME' rg 'RESEARCH_SNAPSHOT_S3_BUCKET_NAME' rg 'FAST_SYNC_SNAPSHOT_S3_BUCKET_NAME'Length of output: 2292
Verification successful
The search results show the calls to
checkIfS3ObjectExistsin bothupdate-research-environment.tsandtake-fast-sync-snapshot.ts. However, the provided context does not include the full function call, so we cannot confirm whether the newbucketparameter is being passed to the function. To ensure that the calls to the function are correct, we need to see the complete lines of code where the function is called.Given the incomplete context, we should request the full lines of code for the calls to
checkIfS3ObjectExiststo verify that thebucketparameter is included.
The output from the script still does not provide the complete lines of code where the
checkIfS3ObjectExistsfunction is called. Without the full lines, we cannot verify if thebucketparameter is being passed correctly in these calls.To resolve this, we need to expand the context of the search to include the lines after the function call, which should contain the parameters passed to the function.
The expanded search results now provide the full lines of code for the calls to
checkIfS3ObjectExistsin bothupdate-research-environment.tsandtake-fast-sync-snapshot.ts. From the output, we can see that thebucketparameter is indeed being passed to the function:
- In
update-research-environment.ts, theRESEARCH_SNAPSHOT_S3_BUCKET_NAMEis passed as thebucketparameter.- In
take-fast-sync-snapshot.ts, theFAST_SYNC_SNAPSHOT_S3_BUCKET_NAMEis passed as thebucketparameter.This confirms that the calls to
checkIfS3ObjectExistshave been updated to include the newbucketparameter as required by the changes in the PR. Therefore, the review comment is verified as correct, and the calls to the function are consistent with the PR objectives.* 150-150: The modification of the `startExportTask` function signature to include a `bucket` parameter is correct and aligns with the PR objectives. Ensure that all calls to this function are updated accordingly.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the modified function signature is correctly used throughout the codebase. rg --type js 'checkIfS3ObjectExists'Length of output: 36
Script:
#!/bin/bash # Broaden the search to include all files and also search for parts of the function name rg 'checkIfS3ObjectExists' -uuLength of output: 1547
Script:
#!/bin/bash # Verify that the calls to `checkIfS3ObjectExists` include the new `bucket` parameter. rg 'await checkIfS3ObjectExists\(' --context 2Length of output: 1035
Script:
#!/bin/bash # Request the full lines of code for the calls to `checkIfS3ObjectExists` to verify the `bucket` parameter inclusion. rg 'await checkIfS3ObjectExists\(' -C 0Length of output: 301
Script:
#!/bin/bash # Expand the context of the search to include the lines after the function call rg 'await checkIfS3ObjectExists\(' -C 3Length of output: 1375
- 224-224: The update to the
OutputLocationin thestartAthenaQueryfunction to useRESEARCH_SNAPSHOT_S3_LOCATION_PREFIXis correct and aligns with the changes made to the constants.indexer/services/roundtable/src/tasks/update-research-environment.ts (4)
18-18: The addition of
RESEARCH_SNAPSHOT_S3_BUCKET_NAMEto the import statement is correct, but it should be checked for proper usage throughout the file.78-85: The new
RESEARCH_SNAPSHOT_S3_BUCKET_NAMEparameter is correctly passed to thecheckIfS3ObjectExistsfunction. Ensure that the new S3 bucket name is configured correctly in the environment and that the bucket exists.85-85: The update to
rdsExportIdentifieris correct. Verify that this new naming convention does not affect any existing processes or scripts that rely on the old naming convention.Verification successful
The review of the
rdsExportIdentifierusage across the codebase indicates that the new naming convention is consistently applied and aligns with the PR's objectives. No issues were found related to this change.* 117-121: The `RESEARCH_SNAPSHOT_S3_BUCKET_NAME` parameter is correctly passed to the `startExportTask` function. Confirm that the function's implementation is updated to handle the new parameter.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the usage of `rdsExportIdentifier` to ensure the new naming convention is handled. rg "rdsExportIdentifier" --files-with-matchesLength of output: 1498
Script:
#!/bin/bash # Check the usage of `rdsExportIdentifier` in the listed files to ensure the new naming convention is correctly implemented. rg "rdsExportIdentifier" --context 2Length of output: 23713
Verification successful
The
startExportTaskfunction in theaws.tshelper file has been updated to include a new parameterbucket, which corresponds to theRESEARCH_SNAPSHOT_S3_BUCKET_NAMEparameter passed in theupdate-research-environment.tsfile. The function signature now accepts this parameter and uses it within therds.startExportTaskcall asS3BucketName.
- The function is correctly updated to handle the new
bucketparameter.- The
S3BucketNamein theawsResponseobject is set using thebucketparameter, which aligns with the changes mentioned in the PR objectives.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the `startExportTask` function is updated to handle the new parameter. rg "startExportTask" --files-with-matchesLength of output: 378
Script:
#!/bin/bash # Examine the `startExportTask` function definition in the `aws.ts` helper file. rg "function startExportTask" -A 10 indexer/services/roundtable/src/helpers/aws.tsLength of output: 513
| @@ -40,6 +41,7 @@ export const configSchema = { | |||
| LOOPS_ORDERBOOK_INSTRUMENTATION: parseBoolean({ default: true }), | |||
| LOOPS_CANCEL_STALE_ORDERS: parseBoolean({ default: true }), | |||
| LOOPS_ENABLED_UPDATE_RESEARCH_ENVIRONMENT: parseBoolean({ default: false }), | |||
| LOOPS_ENABLED_TAKE_FAST_SYNC_SNAPSHOTS: parseBoolean({ default: false }), | |||
There was a problem hiding this comment.
The take-fast-sync-snapshot.ts file exists in the codebase, but the LOOPS_ENABLED_TAKE_FAST_SYNC_SNAPSHOTS flag is not being used within it. This suggests that the flag may not be implemented correctly to control the execution of the snapshot task as intended by the PR objectives. It is recommended to verify the implementation of this flag in the snapshot task logic.
Analysis chain
The addition of the LOOPS_ENABLED_TAKE_FAST_SYNC_SNAPSHOTS configuration flag is correct and aligns with the PR objectives. Ensure that this flag is used correctly throughout the codebase.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the new configuration flag is correctly used throughout the codebase.
rg --type js 'LOOPS_ENABLED_TAKE_FAST_SYNC_SNAPSHOTS'
Length of output: 53
Script:
#!/bin/bash
# Locate the take-fast-sync-snapshot.ts file and check for the usage of the configuration flag.
fd 'take-fast-sync-snapshot.ts' --exec rg 'LOOPS_ENABLED_TAKE_FAST_SYNC_SNAPSHOTS' {}
Length of output: 85
Script:
#!/bin/bash
# Verify the existence of the take-fast-sync-snapshot.ts file in the codebase.
fd 'take-fast-sync-snapshot.ts'
Length of output: 96
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- indexer/services/roundtable/src/index.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- indexer/services/roundtable/src/index.ts
…wl/fs_db_snapshots
…wl/fs_db_snapshots
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- indexer/services/roundtable/src/config.ts (4 hunks)
- indexer/services/roundtable/src/index.ts (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- indexer/services/roundtable/src/config.ts
- indexer/services/roundtable/src/index.ts
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (5)
- indexer/services/roundtable/tests/tasks/take-fast-sync-snapshot.test.ts (1 hunks)
- indexer/services/roundtable/src/config.ts (4 hunks)
- indexer/services/roundtable/src/helpers/aws.ts (4 hunks)
- indexer/services/roundtable/src/tasks/take-fast-sync-snapshot.ts (1 hunks)
- indexer/services/roundtable/src/tasks/update-research-environment.ts (4 hunks)
Files skipped from review as they are similar to previous changes (4)
- indexer/services/roundtable/tests/tasks/take-fast-sync-snapshot.test.ts
- indexer/services/roundtable/src/config.ts
- indexer/services/roundtable/src/tasks/take-fast-sync-snapshot.ts
- indexer/services/roundtable/src/tasks/update-research-environment.ts
Additional comments: 4
indexer/services/roundtable/src/helpers/aws.ts (4)
19-21: The constants
RESEARCH_SNAPSHOT_S3_BUCKET_NAMEandFAST_SYNC_SNAPSHOT_S3_BUCKET_NAMEare derived from the ARN in the config. Ensure that the ARN format is consistent and includes the bucket name after the ':::' delimiter.93-97: The
checkIfS3ObjectExistsfunction now takes abucketparameter, which is used to specify the S3 bucket. Ensure that all calls to this function have been updated to pass the correct bucket name.Verification successful
The script executed to find calls to
checkIfS3ObjectExistswith only two parameters did not produce any output. This suggests that there are no instances in the code where the function is called without the newbucketparameter. Therefore, it seems that all calls to this function have been updated to include thebucketparameter as required.* 191-200: The `startExportTask` function now correctly uses the `bucket` parameter for the `S3BucketName`. Ensure that all calls to this function have been updated to pass the correct bucket name.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to checkIfS3ObjectExists without the bucket parameter. ast-grep --lang javascript --pattern $'checkIfS3ObjectExists($_, $_)'Length of output: 70
Verification successful
The script intended to find calls to
startExportTaskwith only two parameters did not return any results. This could mean that all calls tostartExportTaskhave been updated to include the newbucketparameter. However, to confirm this, we should run a script that searches for calls tostartExportTaskwith three parameters to ensure that thebucketparameter is indeed being passed.
The absence of results from the ast-grep search for calls to
startExportTaskwith both two and three parameters suggests that there may be no direct calls to this function in the codebase, or the function may be called in a way that is not captured by the provided patterns. To ensure thorough verification, we should attempt a more general search that captures any calls tostartExportTask, regardless of the number of parameters.
The search results indicate that the
startExportTaskfunction is called in two different files:update-research-environment.tsandtake-fast-sync-snapshot.ts. Since the function definition inaws.tshas been updated to include abucketparameter, we need to ensure that the calls in these two files have been updated accordingly to pass thebucketparameter.To verify this, we should inspect the calls in the mentioned files to confirm that they are passing the
bucketparameter correctly.
The output from the script shows that the
startExportTaskfunction is called in two places with the correct number of parameters, including thebucketparameter:
- In
update-research-environment.ts, the function is called withRESEARCH_SNAPSHOT_S3_BUCKET_NAMEas the bucket parameter.- In
take-fast-sync-snapshot.ts, the function is called withFAST_SYNC_SNAPSHOT_S3_BUCKET_NAMEas the bucket parameter.This confirms that the calls to
startExportTaskhave been updated to pass the correct bucket name, in line with the changes made to the function's signature.* 268-268: The `OutputLocation` in the `startAthenaQuery` function now uses `RESEARCH_SNAPSHOT_S3_LOCATION_PREFIX`. Verify that this change is consistent with the intended use of the research snapshot S3 bucket and that the prefix is correctly configured in the environment.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to startExportTask without the bucket parameter. ast-grep --lang javascript --pattern $'startExportTask($_, $_)'Length of output: 64
Script:
#!/bin/bash # Search for calls to startExportTask with the bucket parameter. ast-grep --lang javascript --pattern $'startExportTask($_, $_, $_)'Length of output: 68
Script:
#!/bin/bash # Search for any calls to startExportTask to ensure it is being called correctly. rg 'startExportTask\('Length of output: 481
Script:
#!/bin/bash # Inspect the calls to startExportTask in update-research-environment.ts and take-fast-sync-snapshot.ts rg 'startExportTask\(' --files-with-matches | xargs catLength of output: 19232
| let snapshots: RDS.DBSnapshotList = awsResponse.DBSnapshots; | ||
| // Only include snapshots with snapshot identifier that starts with prefixInclude | ||
| if (snapshotIdentifierPrefixInclude !== undefined) { | ||
| snapshots = awsResponse.DBSnapshots | ||
| .filter((snapshot) => snapshot.DBSnapshotIdentifier && | ||
| snapshot.DBSnapshotIdentifier.startsWith(snapshotIdentifierPrefixInclude), | ||
| ); | ||
| } | ||
| if (snapshotIdentifierPrefixExclude !== undefined) { | ||
| snapshots = awsResponse.DBSnapshots | ||
| .filter((snapshot) => snapshot.DBSnapshotIdentifier && | ||
| !snapshot.DBSnapshotIdentifier.startsWith(snapshotIdentifierPrefixExclude), | ||
| ); | ||
| } |
There was a problem hiding this comment.
The filtering logic for snapshotIdentifierPrefixInclude and snapshotIdentifierPrefixExclude uses the same awsResponse.DBSnapshots list, which could lead to incorrect filtering if both includes and excludes are provided. This should be a sequential filter, not parallel.
- snapshots = awsResponse.DBSnapshots
+ snapshots = snapshotsCommittable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| let snapshots: RDS.DBSnapshotList = awsResponse.DBSnapshots; | |
| // Only include snapshots with snapshot identifier that starts with prefixInclude | |
| if (snapshotIdentifierPrefixInclude !== undefined) { | |
| snapshots = awsResponse.DBSnapshots | |
| .filter((snapshot) => snapshot.DBSnapshotIdentifier && | |
| snapshot.DBSnapshotIdentifier.startsWith(snapshotIdentifierPrefixInclude), | |
| ); | |
| } | |
| if (snapshotIdentifierPrefixExclude !== undefined) { | |
| snapshots = awsResponse.DBSnapshots | |
| .filter((snapshot) => snapshot.DBSnapshotIdentifier && | |
| !snapshot.DBSnapshotIdentifier.startsWith(snapshotIdentifierPrefixExclude), | |
| ); | |
| } | |
| let snapshots: RDS.DBSnapshotList = awsResponse.DBSnapshots; | |
| // Only include snapshots with snapshot identifier that starts with prefixInclude | |
| if (snapshotIdentifierPrefixInclude !== undefined) { | |
| snapshots = snapshots | |
| .filter((snapshot) => snapshot.DBSnapshotIdentifier && | |
| snapshot.DBSnapshotIdentifier.startsWith(snapshotIdentifierPrefixInclude), | |
| ); | |
| } | |
| if (snapshotIdentifierPrefixExclude !== undefined) { | |
| snapshots = snapshots | |
| .filter((snapshot) => snapshot.DBSnapshotIdentifier && | |
| !snapshot.DBSnapshotIdentifier.startsWith(snapshotIdentifierPrefixExclude), | |
| ); | |
| } |
| export async function createDBSnapshot( | ||
| rds: RDS, | ||
| snapshotIdentifier: string, | ||
| dbInstanceIdentifier: string, | ||
| ): Promise<string> { | ||
| const params = { | ||
| DBInstanceIdentifier: dbInstanceIdentifier, | ||
| DBSnapshotIdentifier: snapshotIdentifier, | ||
| }; | ||
|
|
||
| const awsResponse: RDS.CreateDBSnapshotResult = await rds.createDBSnapshot(params).promise(); | ||
| if (awsResponse.DBSnapshot === undefined) { | ||
| throw Error(`No DB snapshot was created with identifier: ${snapshotIdentifier}`); | ||
| } | ||
| return awsResponse.DBSnapshot.DBSnapshotIdentifier!; |
There was a problem hiding this comment.
The createDBSnapshot function should have error handling for the createDBSnapshot call to RDS, similar to the error handling in the checkIfS3ObjectExists function.
+ try {
const awsResponse: RDS.CreateDBSnapshotResult = await rds.createDBSnapshot(params).promise();
+ } catch (error) {
+ logger.error({
+ at: `${atStart}createDBSnapshot`,
+ message: 'Failed to create DB snapshot',
+ error,
+ snapshotIdentifier,
+ });
+ throw error;
+ }Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| export async function createDBSnapshot( | |
| rds: RDS, | |
| snapshotIdentifier: string, | |
| dbInstanceIdentifier: string, | |
| ): Promise<string> { | |
| const params = { | |
| DBInstanceIdentifier: dbInstanceIdentifier, | |
| DBSnapshotIdentifier: snapshotIdentifier, | |
| }; | |
| const awsResponse: RDS.CreateDBSnapshotResult = await rds.createDBSnapshot(params).promise(); | |
| if (awsResponse.DBSnapshot === undefined) { | |
| throw Error(`No DB snapshot was created with identifier: ${snapshotIdentifier}`); | |
| } | |
| return awsResponse.DBSnapshot.DBSnapshotIdentifier!; | |
| export async function createDBSnapshot( | |
| rds: RDS, | |
| snapshotIdentifier: string, | |
| dbInstanceIdentifier: string, | |
| ): Promise<string> { | |
| const params = { | |
| DBInstanceIdentifier: dbInstanceIdentifier, | |
| DBSnapshotIdentifier: snapshotIdentifier, | |
| }; | |
| try { | |
| const awsResponse: RDS.CreateDBSnapshotResult = await rds.createDBSnapshot(params).promise(); | |
| } catch (error) { | |
| logger.error({ | |
| at: `${atStart}createDBSnapshot`, | |
| message: 'Failed to create DB snapshot', | |
| error, | |
| snapshotIdentifier, | |
| }); | |
| throw error; | |
| } | |
| if (awsResponse.DBSnapshot === undefined) { | |
| throw Error(`No DB snapshot was created with identifier: ${snapshotIdentifier}`); | |
| } | |
| return awsResponse.DBSnapshot.DBSnapshotIdentifier!; |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- indexer/services/roundtable/tests/tasks/take-fast-sync-snapshot.test.ts (1 hunks)
- indexer/services/roundtable/src/config.ts (4 hunks)
- indexer/services/roundtable/src/helpers/aws.ts (4 hunks)
- indexer/services/roundtable/src/tasks/take-fast-sync-snapshot.ts (1 hunks)
- indexer/services/roundtable/src/tasks/update-research-environment.ts (4 hunks)
Files skipped from review as they are similar to previous changes (4)
- indexer/services/roundtable/tests/tasks/take-fast-sync-snapshot.test.ts
- indexer/services/roundtable/src/config.ts
- indexer/services/roundtable/src/helpers/aws.ts
- indexer/services/roundtable/src/tasks/take-fast-sync-snapshot.ts
Additional comments: 5
indexer/services/roundtable/src/tasks/update-research-environment.ts (5)
- 15-19: The addition of
RESEARCH_SNAPSHOT_S3_BUCKET_NAMEto the import statement suggests that this constant is being used in the file. Ensure that this constant is defined and exported from the../helpers/awsmodule as expected.Verification successful
The search across the codebase has confirmed that the constant
RESEARCH_SNAPSHOT_S3_BUCKET_NAMEis indeed defined and exported, as it was found in one of the files. This aligns with the changes mentioned in the PR summary and the review comment.
- The constant
RESEARCH_SNAPSHOT_S3_BUCKET_NAMEis present and exported in the codebase.* 69-73: The `getMostRecentDBSnapshotIdentifier` function is now called with `config.FAST_SYNC_SNAPSHOT_IDENTIFIER_PREFIX`. Verify that this configuration value is correctly set in the `config.ts` file and that it is consistent with the intended use of the function.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that RESEARCH_SNAPSHOT_S3_BUCKET_NAME is defined and exported in the aws helper module. rg --type ts "export const RESEARCH_SNAPSHOT_S3_BUCKET_NAME" src/helpers/aws.tsLength of output: 140
Script:
#!/bin/bash # Find the file that defines and exports RESEARCH_SNAPSHOT_S3_BUCKET_NAME within the helpers directory fd "aws" src/helpers --exec rg "export const RESEARCH_SNAPSHOT_S3_BUCKET_NAME"Length of output: 177
Script:
#!/bin/bash # Search for the file that defines and exports RESEARCH_SNAPSHOT_S3_BUCKET_NAME in the entire codebase fd "aws" --exec rg "export const RESEARCH_SNAPSHOT_S3_BUCKET_NAME"Length of output: 170
Verification successful
The
FAST_SYNC_SNAPSHOT_IDENTIFIER_PREFIXis indeed defined in theindexer/services/roundtable/src/config.tsfile with a default value of'fast-sync'. This confirms that the configuration value is correctly set and is consistent with the intended use in thegetMostRecentDBSnapshotIdentifierfunction call.* 82-86: The `checkIfS3ObjectExists` function has been updated to include the `RESEARCH_SNAPSHOT_S3_BUCKET_NAME` parameter. Ensure that the function definition is updated accordingly and that the parameter is being used correctly within the function.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that FAST_SYNC_SNAPSHOT_IDENTIFIER_PREFIX is defined in the config module. rg --type ts "FAST_SYNC_SNAPSHOT_IDENTIFIER_PREFIX" src/config.tsLength of output: 119
Script:
#!/bin/bash # Search for the config.ts file in the repository to verify its existence. fd config.tsLength of output: 834
Script:
#!/bin/bash # Verify that FAST_SYNC_SNAPSHOT_IDENTIFIER_PREFIX is defined in the roundtable service's config module. rg --type ts "FAST_SYNC_SNAPSHOT_IDENTIFIER_PREFIX" indexer/services/roundtable/src/config.tsLength of output: 172
Verification successful
The
checkIfS3ObjectExistsfunction definition has been successfully located and it confirms that the function has been updated to include a new parameterbucket, which corresponds to theRESEARCH_SNAPSHOT_S3_BUCKET_NAMEparameter mentioned in the review comment.
- The function signature now accepts three parameters:
s3of typeS3,s3Dateof typestring, andbucketof typestring.- The function returns a
Promise<boolean>.Based on the provided output, the review comment has been addressed correctly in the codebase.
* 89-89: The `rdsExportIdentifier` string now includes a "research-" prefix. Confirm that this change is consistent with the intended naming conventions and that it does not conflict with any existing identifiers in the system.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that checkIfS3ObjectExists function is updated to accept the new parameter. rg --type ts "function checkIfS3ObjectExists" src/helpers/aws.tsLength of output: 125
Script:
#!/bin/bash # Search for the file that contains the checkIfS3ObjectExists function definition. rg --type ts "function checkIfS3ObjectExists"Length of output: 139
Script:
#!/bin/bash # Extract the definition of the checkIfS3ObjectExists function to verify the inclusion of the new parameter. rg --type ts -A 10 "function checkIfS3ObjectExists" indexer/services/roundtable/src/helpers/aws.tsLength of output: 453
Verification successful
The search results indicate that the
rdsExportIdentifieris used in various places within the codebase, particularly in thetake-fast-sync-snapshot.tsandupdate-research-environment.tsfiles, as well as in several helper and library files. The presence of the "research-" prefix in theupdate-research-environment.tsfile suggests that this identifier is specifically for research-related snapshots, which seems to be distinct from the fast sync snapshots mentioned intake-fast-sync-snapshot.ts.Given that the identifier is used in the context of both research and fast sync snapshots, and the prefix "research-" is only added in the
update-research-environment.tsfile, it appears that the naming convention is intended to differentiate between these two types of snapshots. There is no direct evidence from the search results that the new naming convention conflicts with existing identifiers, as the prefix "research-" seems to be a deliberate addition to distinguish between different snapshot types.Based on the information provided, the change to the
rdsExportIdentifierstring to include a "research-" prefix seems consistent with the intended naming conventions and does not show any immediate conflicts with existing identifiers in the system.* 121-126: The `startExportTask` function has been updated to include the `RESEARCH_SNAPSHOT_S3_BUCKET_NAME` parameter and a boolean value `true`. Verify that the function definition is updated to handle these parameters and that the boolean value is used appropriately within the function.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the new rdsExportIdentifier naming convention does not conflict with existing identifiers. rg --type ts "rdsExportIdentifier" -- src/Length of output: 87
Script:
#!/bin/bash # Search for occurrences of "rdsExportIdentifier" throughout the entire repository to check for conflicts. rg --type ts "rdsExportIdentifier"Length of output: 6831
Verification successful
The script has successfully located the
startExportTaskfunction, but the output provided is incomplete and does not show the full function signature or its parameters. To verify the review comment, we need the complete function signature and the relevant parts of the function body to ensure that the new parameters are handled correctly.Let's generate a new script to get the full function signature and the relevant parts of the function body.
The output from the script provides the full signature of the
startExportTaskfunction, confirming that it has been updated to include the new parameters:bucketas a string andisAutomatedSnapshotas a boolean. This matches the usage seen in the provided snippet from theupdate-research-environment.tsfile, whereRESEARCH_SNAPSHOT_S3_BUCKET_NAMEis passed as thebucketparameter andtrueas theisAutomatedSnapshotparameter.Based on the information provided, the
startExportTaskfunction definition has been correctly updated to handle these parameters.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that startExportTask function is updated to accept the new parameters. rg --type ts "function startExportTask" src/helpers/aws.tsLength of output: 119
Script:
#!/bin/bash # Find the file that contains the definition of startExportTask function. fd "aws.ts" --type f --exec rg "function startExportTask" {}Length of output: 101
Script:
#!/bin/bash # Retrieve the full function signature and the first few lines of the function body for startExportTask. fd "aws.ts" --type f --exec rg -A 5 "function startExportTask" {}Length of output: 228
…wl/fs_db_snapshots
| rds: RDS, | ||
| snapshotIdentifierPrefixInclude?: string, | ||
| snapshotIdentifierPrefixExclude?: string, | ||
| ): Promise<string | undefined> { | ||
| const awsResponse: RDS.DBSnapshotMessage = await rds.describeDBSnapshots({ | ||
| DBInstanceIdentifier: config.RDS_INSTANCE_NAME, | ||
| MaxRecords: 20, // this is the minimum |
There was a problem hiding this comment.
There is a bug (noted in TODO(CLOB-672)) where if there are more than 20 DB snapshots for the RDS instance, this method won't return the latest DB snapshot. The fix (which wasn't implemented yet) is to page through all the snapshots and then get the latest one.
As such I don't think this will work correctly in any of the long-running test-nets (public testnet / mainnet).
| // Polling function to check snapshot status. Only return when the snapshot is available. | ||
| const waitForSnapshot = async () => { | ||
| // eslint-disable-next-line no-constant-condition | ||
| while (true) { |
There was a problem hiding this comment.
nit: Add some max wait time where the task breaks out of this loop, and some logs indicating how long the task is waiting for the snapshot at some X interval to help with debugging if this loops somehow never terminates.
There was a problem hiding this comment.
changed to use waiter in AWS SDK, that sets a max time limit (retries * delay)
| try { | ||
| await rds.createDBSnapshot(params).promise(); | ||
| // Polling function to check snapshot status. Only return when the snapshot is available. | ||
| const waitForSnapshot = async () => { |
There was a problem hiding this comment.
nit: I think you can use this waiter method/object that is part of the AWS sdk here rather than writing your own. Non-blocking though, if it's too complicated to use, no need to use it.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- indexer/services/roundtable/tests/tasks/take-fast-sync-snapshot.test.ts (1 hunks)
- indexer/services/roundtable/src/config.ts (4 hunks)
- indexer/services/roundtable/src/helpers/aws.ts (2 hunks)
- indexer/services/roundtable/src/tasks/take-fast-sync-snapshot.ts (1 hunks)
- indexer/services/roundtable/src/tasks/update-research-environment.ts (1 hunks)
Additional comments: 10
indexer/services/roundtable/__tests__/tasks/take-fast-sync-snapshot.test.ts (1)
- 9-50: The test cases seem to cover the scenarios where a snapshot should be taken and when it should not. However, it's important to ensure that the mock implementations for
getMostRecentDBSnapshotIdentifierandcreateDBSnapshotaccurately reflect the conditions being tested, and that the assertions correctly validate the behavior oftakeFastSyncSnapshotTask.Verification successful
The previous script contained an error due to the incorrect usage of the
--execflag withrg. Let's correct the script to properly search for the usage ofjest.mockand the assertions related tocreateDBSnapshot.
The output from the scripts confirms that the
take-fast-sync-snapshot.test.tsfile contains the mocked implementations ofgetMostRecentDBSnapshotIdentifierand the assertions oncreateDBSnapshot. The tests are checking both scenarios wherecreateDBSnapshotshould be called and where it should not be called, based on the conditions set up in the mocks.The test cases in
take-fast-sync-snapshot.test.tsare as follows:
- A snapshot is expected to be taken if the last snapshot was more than the interval ago (
expect(createDBSnapshot).toHaveBeenCalled()).- No snapshot is expected to be taken if the last snapshot was less than the interval ago (
expect(createDBSnapshot).not.toHaveBeenCalled()).- A snapshot is expected to be taken if there is no existing snapshot (
expect(createDBSnapshot).toHaveBeenCalled()).These assertions correspond to the intended behavior of the
takeFastSyncSnapshotTaskfunction under different conditions, as described in the review comment. Therefore, the test cases appear to be correctly validating the behavior of the task.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the mock implementations in the test cases reflect the actual behavior of the functions. rg --type ts --files-with-matches 'jest.mock\(' --exec cat {} | grep -E 'getMostRecentDBSnapshotIdentifier|createDBSnapshot' # Ensure that the assertions in the test cases are correctly validating the behavior of the task. rg --type ts --files-with-matches 'expect\(createDBSnapshot\)' --exec cat {} | grep -E 'toHaveBeenCalled|not.toHaveBeenCalled'Length of output: 1077
Script:
#!/bin/bash # Find files that contain jest.mock and check for the mocked functions. rg --type ts 'jest.mock\(' --files-with-matches | xargs rg 'getMostRecentDBSnapshotIdentifier|createDBSnapshot' # Find files that contain expectations on createDBSnapshot and check the assertions. rg --type ts 'expect\(createDBSnapshot\)' --files-with-matches | xargs rg 'toHaveBeenCalled|not.toHaveBeenCalled'Length of output: 1688
indexer/services/roundtable/src/tasks/take-fast-sync-snapshot.ts (1)
- 1-78: The logic for determining whether to take a new snapshot based on the interval since the last snapshot appears to be correct. However, it's important to ensure that the date parsing and comparison logic in
isDifferenceLessThanIntervalis robust and accounts for any edge cases, such as leap years or daylight saving time changes.Verification successful
The previous script contained an error due to the misuse of the
--execflag withrg. I will correct this and generate a new script to verify the date parsing and snapshot creation logic.
The script execution failed because the output from
rgwas not properly formatted for use withxargsandcat. I will adjust the script to correctly extract the filenames before attempting to display their contents.
The verification process has provided the following information:
- The date parsing logic in the
isDifferenceLessThanIntervalfunction usesnew Date(year, month, day, hour, minute)to create a new Date object. This approach should handle standard date and time edge cases, but it's important to note that JavaScript Date objects have zero-based months, which could lead to errors if not accounted for in the parsing logic.- The
DateTime.utc().toFormat('yyyy-MM-dd-HH-mm')line indicates that the current date and time are being formatted correctly in UTC, which is a good practice for consistent time comparisons across different time zones.- The snapshot creation logic includes a check
if (lastSnapshotIdentifier !== undefined)to determine whether a snapshot already exists before proceeding with the creation of a new one.Based on the output, the code appears to handle the date parsing and snapshot creation logic correctly, with the caveat that the zero-based month indexing in JavaScript's Date object needs to be considered in the parsing function. If this is handled correctly within the
parseDateStringfunction, then the logic is sound.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the date parsing handles edge cases correctly. rg --type ts --files-with-matches 'isDifferenceLessThanInterval' --exec cat {} | grep -E 'new Date|DateTime.utc' # Ensure that the snapshot creation logic correctly handles the case where no snapshot exists. rg --type ts --files-with-matches 'createDBSnapshot' --exec cat {} | grep -E 'if \(lastSnapshotIdentifier !== undefined\)'Length of output: 1057
Script:
#!/bin/bash # Verify that the date parsing handles edge cases correctly. rg --type ts 'isDifferenceLessThanInterval' | xargs -I {} cat {} | grep -E 'new Date|DateTime.utc' # Ensure that the snapshot creation logic correctly handles the case where no snapshot exists. rg --type ts 'createDBSnapshot' | xargs -I {} cat {} | grep -E 'if \(lastSnapshotIdentifier !== undefined\)'Length of output: 1832
Script:
#!/bin/bash # Verify that the date parsing handles edge cases correctly. rg --type ts 'isDifferenceLessThanInterval' -l | xargs cat | grep -E 'new Date|DateTime.utc' # Ensure that the snapshot creation logic correctly handles the case where no snapshot exists. rg --type ts 'createDBSnapshot' -l | xargs cat | grep -E 'if \(lastSnapshotIdentifier !== undefined\)'Length of output: 368
indexer/services/roundtable/src/config.ts (4)
18-18: The addition of the
FOUR_HOURS_IN_MILLISECONDSconstant is correct and aligns with the PR objectives to take snapshots every four hours.44-44: The
LOOPS_ENABLED_TAKE_FAST_SYNC_SNAPSHOTSconfiguration option is added to enable the snapshot task. Ensure that this flag is used correctly throughout the codebase.Verification successful
The
LOOPS_ENABLED_TAKE_FAST_SYNC_SNAPSHOTSconfiguration option is used in theindex.tsfile to conditionally start the snapshot task loop. This aligns with the intended use of the flag as per the PR objectives and review comment.
* 71-73: The `LOOPS_INTERVAL_MS_TAKE_FAST_SYNC_SNAPSHOTS` configuration setting is added to set the interval for the snapshot task. This should match the interval used in the task logic.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the new configuration flag is correctly used throughout the codebase. rg --type ts 'LOOPS_ENABLED_TAKE_FAST_SYNC_SNAPSHOTS'Length of output: 267
Verification successful
The search results confirm that the
LOOPS_INTERVAL_MS_TAKE_FAST_SYNC_SNAPSHOTSconfiguration setting is indeed used within the task logic:
- In
index.ts, it is passed as a parameter tostartLoopwhich likely controls the interval at which thetakeFastSyncSnapshotTaskis executed.- In
take-fast-sync-snapshot.ts, it is used within theisDifferenceLessThanIntervalfunction to determine if the last snapshot was taken less than the interval ago, and it is also logged to indicate the interval at which the last fast sync database snapshot was taken.These findings verify that the
LOOPS_INTERVAL_MS_TAKE_FAST_SYNC_SNAPSHOTSsetting is used in the task logic as intended.* 120-120: The `FAST_SYNC_SNAPSHOT_IDENTIFIER_PREFIX` configuration is added to differentiate between snapshot types. Confirm that this prefix is used consistently in the snapshot creation and retrieval logic.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the interval setting is used in the task logic. rg --type ts 'LOOPS_INTERVAL_MS_TAKE_FAST_SYNC_SNAPSHOTS' --exec cat {} | grep -E 'takeFastSyncSnapshotTask|isDifferenceLessThanInterval'Length of output: 548
Script:
#!/bin/bash # Search for the usage of 'LOOPS_INTERVAL_MS_TAKE_FAST_SYNC_SNAPSHOTS' within TypeScript files # and check if it's used in the task logic, specifically in the 'takeFastSyncSnapshotTask' function. rg --type ts 'LOOPS_INTERVAL_MS_TAKE_FAST_SYNC_SNAPSHOTS' -C 3Length of output: 2407
Verification successful
The search results show that the
FAST_SYNC_SNAPSHOT_IDENTIFIER_PREFIXis used in the following contexts:
- Defined in
config.tswith a default value of 'fast-sync'.- Used in
take-fast-sync-snapshot.tsto construct a snapshot identifier string.- Passed as an argument in
take-fast-sync-snapshot.tsandupdate-research-environment.ts, likely to functions that handle snapshot creation and retrieval.- Referenced in the tests within
take-fast-sync-snapshot.test.tsto construct a snapshot identifier for testing purposes.These findings indicate that the
FAST_SYNC_SNAPSHOT_IDENTIFIER_PREFIXis indeed used consistently in the snapshot creation and retrieval logic, as it is included in the construction of snapshot identifiers and passed to relevant functions.Based on the provided context and the results from the executed scripts, the review comment has been addressed correctly in the codebase.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the snapshot identifier prefix is used consistently. rg --type ts 'FAST_SYNC_SNAPSHOT_IDENTIFIER_PREFIX' --exec cat {} | grep -E 'createDBSnapshot|getMostRecentDBSnapshotIdentifier'Length of output: 539
Script:
#!/bin/bash # Search for the usage of FAST_SYNC_SNAPSHOT_IDENTIFIER_PREFIX in relation to createDBSnapshot and getMostRecentDBSnapshotIdentifier functions. rg --type ts 'FAST_SYNC_SNAPSHOT_IDENTIFIER_PREFIX' | grep -E 'createDBSnapshot|getMostRecentDBSnapshotIdentifier'Length of output: 114
Script:
#!/bin/bash # Search for all occurrences of FAST_SYNC_SNAPSHOT_IDENTIFIER_PREFIX to examine its usage. rg --type ts 'FAST_SYNC_SNAPSHOT_IDENTIFIER_PREFIX'Length of output: 996
indexer/services/roundtable/src/tasks/update-research-environment.ts (1)
- 69-73: The modification to the
getMostRecentDBSnapshotIdentifierfunction call includes additional arguments to filter snapshots by identifier prefix. Ensure that these changes are consistent with the intended behavior and that the prefix is used correctly.Verification successful
It seems there was a syntax error in the previous script due to the incorrect usage of the
--execflag withrg. Let's correct this and try to verify the usage ofgetMostRecentDBSnapshotIdentifieragain.--- The output from the script shows several instances where `getMostRecentDBSnapshotIdentifier` is used across the codebase. To verify the changes made to the function call in `update-research-environment.ts`, we need to examine the definition of `getMostRecentDBSnapshotIdentifier` in `aws.ts` to understand the expected parameters and then ensure that the usage in `update-research-environment.ts` aligns with this definition. --- The definition of `getMostRecentDBSnapshotIdentifier` in `aws.ts` shows that the function accepts three parameters: 1. `rds`: An instance of RDS. 2. `snapshotIdentifierPrefixInclude`: An optional string to include snapshots with a certain prefix. 3. `snapshotIdentifierPrefixExclude`: An optional string to exclude snapshots with a certain prefix. The usage in `update-research-environment.ts` provides `rds`, `undefined`, and `config.FAST_SYNC_SNAPSHOT_IDENTIFIER_PREFIX` as arguments, which aligns with the function's definition. The `undefined` argument corresponds to the `snapshotIdentifierPrefixExclude` parameter, which is optional and is being explicitly passed as `undefined` in this case. Based on the provided definition and the usage in the `update-research-environment.ts` file, the changes to the function call are consistent with the intended behavior, and the prefix is used correctly to filter snapshots by identifier. </details> <details> <summary>Scripts Executed</summary> The following scripts were executed for the analysis: --- Script: ```shell #!/bin/bash # Verify that the snapshot identifier prefix is used correctly in the function call. rg --type ts 'getMostRecentDBSnapshotIdentifier' --exec cat {} | grep -E 'rds|undefined|config.FAST_SYNC_SNAPSHOT_IDENTIFIER_PREFIX'Length of output: 543
Script:
#!/bin/bash # Verify the usage of `getMostRecentDBSnapshotIdentifier` and its arguments across the codebase. rg 'getMostRecentDBSnapshotIdentifier' --type tsLength of output: 1697
Script:
#!/bin/bash # Examine the definition of `getMostRecentDBSnapshotIdentifier` in `aws.ts`. cat indexer/services/roundtable/src/helpers/aws.ts | grep -A 10 'export async function getMostRecentDBSnapshotIdentifier'Length of output: 549
indexer/services/roundtable/src/helpers/aws.ts (3)
19-21: The export of
S3_BUCKET_NAMEandS3_LOCATION_PREFIXconstants is correct and aligns with the PR objectives to support snapshot creation and retrieval.42-116: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [31-67]
The
getMostRecentDBSnapshotIdentifierfunction is modified to accept optional parameters for filtering snapshots. Ensure that the logic for filtering and returning the most recent snapshot identifier is correct and that the function handles edge cases properly.Verification successful
The script has successfully retrieved the context around the
getMostRecentDBSnapshotIdentifierfunction from theaws.tsfile. However, the output provided does not include the lines where the filtering logic withfilterandstartsWithis applied. To verify the logic for filtering and returning the most recent snapshot identifier, we need to see the complete function body.Given the partial output, I will attempt another script to retrieve the rest of the function body to verify the logic as requested in the review comment.
The script has successfully retrieved more context around the
getMostRecentDBSnapshotIdentifierfunction from theaws.tsfile, including the start of the filtering logic. However, the output is still incomplete and does not show the entire function body, particularly the part where the filtering withsnapshotIdentifierPrefixExcludeis applied and the final return statement.To fully verify the logic for filtering and returning the most recent snapshot identifier, we need to see the complete function body, including the end of the function where the most recent snapshot is determined and returned.
I will attempt another script to retrieve the remaining part of the function body to verify the logic as requested in the review comment.
The script has now successfully retrieved the complete function body for
getMostRecentDBSnapshotIdentifierfrom theaws.tsfile. Based on the provided output, we can verify the logic for filtering and returning the most recent snapshot identifier:
- The function accepts an RDS client and two optional parameters for snapshot identifier prefixes to include or exclude.
- It retrieves a list of snapshots for the given RDS instance name, with a maximum of 20 records.
- If no snapshots are found, it throws an error.
- It filters the snapshots to only include those with identifiers that start with
snapshotIdentifierPrefixInclude, if provided.- It further filters the snapshots to exclude those with identifiers that start with
snapshotIdentifierPrefixExclude, if provided.- It logs the most recent snapshot.
- It returns the identifier of the most recent snapshot, or
undefinedif there are no snapshots after filtering.The logic appears to correctly handle the inclusion and exclusion of snapshots based on the provided prefixes and returns the most recent snapshot identifier. The function also seems to handle the case where no snapshots are found by throwing an error.
Based on the code provided, the filtering logic and the handling of edge cases in the
getMostRecentDBSnapshotIdentifierfunction are correct.* 73-113: The `createDBSnapshot` function is added to create a DB snapshot and wait for its availability. Confirm that the function correctly handles the creation process and polling for snapshot availability.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the filtering logic in the function is correct. rg --type ts 'getMostRecentDBSnapshotIdentifier' --exec cat {} | grep -E 'filter|startsWith'Length of output: 503
Script:
#!/bin/bash # Correcting the script to properly use rg and grep to verify the filtering logic in the function. rg --type ts 'getMostRecentDBSnapshotIdentifier' | grep -E 'filter|startsWith'Length of output: 78
Script:
#!/bin/bash # Using rg to directly search for the lines containing 'filter' or 'startsWith' within the function 'getMostRecentDBSnapshotIdentifier'. rg --type ts -A 10 'getMostRecentDBSnapshotIdentifier' | rg 'filter|startsWith'Length of output: 79
Script:
#!/bin/bash # Using rg to search for the function definition and display surrounding lines including the filtering logic. rg --type ts -C 10 'async function getMostRecentDBSnapshotIdentifier'Length of output: 1953
Script:
#!/bin/bash # Attempting to retrieve the complete function body for getMostRecentDBSnapshotIdentifier rg --type ts -C 20 'async function getMostRecentDBSnapshotIdentifier' indexer/services/roundtable/src/helpers/aws.tsLength of output: 1740
Script:
#!/bin/bash # Attempting to retrieve the remaining part of the function body for getMostRecentDBSnapshotIdentifier rg --type ts -C 40 'async function getMostRecentDBSnapshotIdentifier' indexer/services/roundtable/src/helpers/aws.tsLength of output: 2586
Verification successful
The provided script output shows the relevant section of the
aws.tsfile containing thecreateDBSnapshotfunction. The function includes a polling mechanism that uses awhile (true)loop to repeatedly check the status of the snapshot until it becomes available or fails. The loop waits for 1 minute before each check.Based on the output, the
createDBSnapshotfunction appears to handle the creation process and polling for snapshot availability as described in the review comment. However, the output is incomplete, and the rest of the function, including error handling and the condition for the snapshot status 'failed', is not shown.To fully verify the correctness of the
createDBSnapshotfunction, we need to see the entire function, including the loop and any error handling that follows the loop. Let's run another script to get the complete function definition.
The complete
createDBSnapshotfunction from theaws.tsfile has been provided. The function includes the following logic:
- It takes
rds,snapshotIdentifier, anddbInstanceIdentifieras parameters.- It constructs the parameters for the
createDBSnapshotAPI call and awaits its promise.- It defines a
waitForSnapshotasynchronous function that enters awhile (true)loop to poll the snapshot status.- Inside the loop, it calls
describeDBSnapshotsand checks the status of the snapshot:
- If the status is
'available', it returns the snapshot identifier.- If the status is
'failed', it throws an error indicating snapshot creation failure.- The loop includes a 1-minute wait time before each status check iteration.
- If an error occurs during the snapshot creation or polling process, it logs the error and rethrows it.
The function appears to correctly handle the creation process and polling for snapshot availability, as it waits for the snapshot to become available before returning and handles possible failure states.
Based on the provided script output, the review comment regarding the
createDBSnapshotfunction is consistent with the codebase. The function is designed to create a DB snapshot and wait for its availability, with appropriate error handling and polling logic.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the snapshot creation and polling logic is correct. rg --type ts 'createDBSnapshot' --exec cat {} | grep -E 'createDBSnapshot|while \(true\)'Length of output: 500
Script:
#!/bin/bash # Correcting the script to properly display the contents of the file where `createDBSnapshot` is defined. rg --type ts 'export async function createDBSnapshot' -C 20Length of output: 3359
Script:
#!/bin/bash # Display the complete `createDBSnapshot` function to verify the snapshot creation and polling logic. rg --type ts 'export async function createDBSnapshot' -C 50Length of output: 8480
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- indexer/services/roundtable/src/helpers/aws.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- indexer/services/roundtable/src/helpers/aws.ts
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- indexer/services/roundtable/src/helpers/aws.ts (1 hunks)
- indexer/services/roundtable/src/tasks/take-fast-sync-snapshot.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- indexer/services/roundtable/src/helpers/aws.ts
- indexer/services/roundtable/src/tasks/take-fast-sync-snapshot.ts
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- indexer/services/roundtable/tests/tasks/take-fast-sync-snapshot.test.ts (1 hunks)
- indexer/services/roundtable/src/config.ts (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- indexer/services/roundtable/tests/tasks/take-fast-sync-snapshot.test.ts
- indexer/services/roundtable/src/config.ts
…wl/fs_db_snapshots
| // Filter for fast sync snapshots older than cutoffTime | ||
| const oldFastSyncSnapshots = response.DBSnapshots.filter((snapshot) => { | ||
| if (!snapshot.DBSnapshotIdentifier!.startsWith( | ||
| config.FAST_SYNC_SNAPSHOT_IDENTIFIER_PREFIX, |
There was a problem hiding this comment.
nit: can we log an error here? Technically there shouldn't be anything that doesn't match our fast sync identifier right?
There was a problem hiding this comment.
the rds automated daily snapshots will also be included here
| * @param endDate | ||
| * @param intervalMs | ||
| */ | ||
| function isDifferenceLessThanInterval( |
There was a problem hiding this comment.
nit: move this under runTask so that the top level function is at the top of the file
| logger.info({ | ||
| at, | ||
| message: 'Last fast sync db snapshot was taken less than the interval ago', | ||
| interval: config.LOOPS_INTERVAL_MS_TAKE_FAST_SYNC_SNAPSHOTS, |
There was a problem hiding this comment.
nit: log the s3Date and dateString
| const startSnapshot: number = Date.now(); | ||
| const createdSnapshotIdentifier: string = await | ||
| createDBSnapshot(rds, snapshotIdentifier, config.RDS_INSTANCE_NAME); | ||
| logger.info({ at, message: 'Created DB snapshot.', snapshotIdentifier: createdSnapshotIdentifier }); | ||
| stats.timing(`${statStart}.createDbSnapshot`, Date.now() - startSnapshot); | ||
| const startDeleteOldSnapshot: number = Date.now(); |
There was a problem hiding this comment.
nit: separate into a separate function
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (6)
- indexer/packages/base/src/constants.ts (1 hunks)
- indexer/services/roundtable/tests/tasks/take-fast-sync-snapshot.test.ts (1 hunks)
- indexer/services/roundtable/src/config.ts (4 hunks)
- indexer/services/roundtable/src/index.ts (2 hunks)
- indexer/services/roundtable/src/tasks/delete-old-fast-sync-snapshots.ts (1 hunks)
- indexer/services/roundtable/src/tasks/take-fast-sync-snapshot.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- indexer/packages/base/src/constants.ts
Additional comments: 8
indexer/services/roundtable/src/tasks/delete-old-fast-sync-snapshots.ts (1)
- 1-19: Ensure that the
deleteOldFastSyncSnapshotsfunction handles exceptions appropriately to avoid unhandled promise rejections.Verification successful
The
deleteOldFastSyncSnapshotsfunction includes a try-catch block for error handling, with appropriate logging in the catch block to handle any exceptions that may occur during the process. This confirms that the function has proper error handling in place.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that deleteOldFastSyncSnapshots has proper error handling ast-grep --lang typescript --pattern $'async function deleteOldFastSyncSnapshots($_) { try { $$$ } catch ($_) { $$$ } }'Length of output: 135
Script:
#!/bin/bash # Search for the definition of deleteOldFastSyncSnapshots to check for error handling rg "async function deleteOldFastSyncSnapshots"Length of output: 192
Script:
#!/bin/bash # Search for try-catch blocks within the deleteOldFastSyncSnapshots function definition rg -A 10 "async function deleteOldFastSyncSnapshots" indexer/services/roundtable/src/helpers/aws.tsLength of output: 582
Script:
#!/bin/bash # Retrieve more lines following the try block to confirm the presence of the catch block rg -A 20 "async function deleteOldFastSyncSnapshots" indexer/services/roundtable/src/helpers/aws.tsLength of output: 966
Script:
#!/bin/bash # Retrieve further lines to confirm the presence and content of the catch block rg -A 30 "async function deleteOldFastSyncSnapshots" indexer/services/roundtable/src/helpers/aws.tsLength of output: 1280
Script:
#!/bin/bash # Retrieve further lines to confirm the presence and content of the catch block rg -A 40 "async function deleteOldFastSyncSnapshots" indexer/services/roundtable/src/helpers/aws.tsLength of output: 1718
Script:
#!/bin/bash # Retrieve further lines to confirm the presence and content of the catch block rg -A 50 "async function deleteOldFastSyncSnapshots" indexer/services/roundtable/src/helpers/aws.tsLength of output: 1997
indexer/services/roundtable/__tests__/tasks/take-fast-sync-snapshot.test.ts (1)
- 1-53: The test cases seem to cover the scenarios well. However, ensure that the mock implementations are returning values that accurately represent the production environment for reliable testing.
indexer/services/roundtable/src/tasks/take-fast-sync-snapshot.ts (1)
- 1-52: The logic for checking the interval and creating a new DB snapshot appears to be correct. Ensure that the
createDBSnapshotfunction has proper error handling to manage any exceptions that may occur during the snapshot creation process.Verification successful
The verification script did not return any output, which suggests that the
createDBSnapshotfunction may not have a try-catch block for error handling within its definition. This could be a potential issue if the function is expected to handle errors internally.To confirm the structure of the
createDBSnapshotfunction and to check for error handling, I will generate a new script to search for the function definition without assuming it contains a try-catch block.
The absence of output from the ast-grep command suggests that the
createDBSnapshotfunction may not be defined in the expected format, or it might not be present in the codebase at all. To further investigate, I will use a different approach to search for the function definition using ripgrep (rg), which can search the codebase without relying on the structure of the function.
The output from the ripgrep command confirms that the
createDBSnapshotfunction is indeed defined in theaws.tsfile. However, the output does not provide details about the function's structure or whether it includes error handling.To get more information about the error handling within the
createDBSnapshotfunction, I will generate a new script to display the contents of the function from theaws.tsfile.
The output from the script shows the beginning of the
createDBSnapshotfunction, including the start of a try block, which indicates that there is some form of error handling within the function. However, the output is truncated and does not show the entire error handling logic.To fully understand the error handling implemented in the
createDBSnapshotfunction, I will generate a new script to display more lines following the detected try block.
The provided output shows the
createDBSnapshotfunction, which includes a try block where the snapshot creation and a wait for the snapshot to become available are initiated. However, the output is still truncated and does not show the catch block that would handle any errors.To complete the verification of the error handling within the
createDBSnapshotfunction, I will generate a new script to display the rest of the function, focusing on the catch block.
The script failed due to a regex parse error, specifically an "unopened group" error. This is likely because the parentheses in the search pattern were interpreted as regex group markers. To correct this, I need to escape the parentheses in the search pattern.
Let's try a new script with the corrected regex pattern to display the rest of the
createDBSnapshotfunction, including the catch block for error handling.
The attempt to correct the regex pattern still resulted in a parse error. To avoid this issue, I will use a simpler pattern that should match the context around the end of the
createDBSnapshotfunction without using special regex characters that need to be escaped.
The output provided shows more of the
createDBSnapshotfunction, including theawaitfor the snapshot to become available and the initiation of a call to retrieve the snapshot details. However, the output is still truncated and does not show the catch block.To ensure we have a complete understanding of the error handling within the
createDBSnapshotfunction, I will generate a new script to display the lines following thedescribeDBSnapshotscall, which should include the catch block.
The new output shows the continuation of the
createDBSnapshotfunction, including the retrieval of the snapshot details and a conditional check on the snapshot's status. It also confirms the presence of a catch block, which is used for error handling.This information verifies that the
createDBSnapshotfunction includes error handling logic to manage exceptions that may occur during the snapshot creation process. The function waits for the snapshot to become available and throws an error if the snapshot is not in the 'available' state, which is then caught in the catch block.Based on the information gathered, the error handling within the
createDBSnapshotfunction is implemented as expected.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that createDBSnapshot has proper error handling ast-grep --lang typescript --pattern $'async function createDBSnapshot($_, $_, $_) { try { $$$ } catch ($_) { $$$ } }'Length of output: 133
Script:
#!/bin/bash # Search for the createDBSnapshot function to verify its structure and error handling ast-grep --lang typescript --pattern $'async function createDBSnapshot($_, $_, $_) { $$$ }'Length of output: 94
Script:
#!/bin/bash # Search for the createDBSnapshot function definition using ripgrep rg "async function createDBSnapshot"Length of output: 125
Script:
#!/bin/bash # Display the contents of the createDBSnapshot function from aws.ts rg -A 10 "async function createDBSnapshot" indexer/services/roundtable/src/helpers/aws.tsLength of output: 353
Script:
#!/bin/bash # Display more lines of the createDBSnapshot function to understand the error handling rg -A 20 "async function createDBSnapshot" indexer/services/roundtable/src/helpers/aws.tsLength of output: 753
Script:
#!/bin/bash # Display the rest of the createDBSnapshot function to see the catch block for error handling rg -A 10 -B 10 "}).promise();" indexer/services/roundtable/src/helpers/aws.tsLength of output: 145
Script:
#!/bin/bash # Display the rest of the createDBSnapshot function to see the catch block for error handling rg -A 10 -B 10 "\}).promise\(\);" indexer/services/roundtable/src/helpers/aws.tsLength of output: 152
Script:
#!/bin/bash # Display the rest of the createDBSnapshot function to see the catch block for error handling rg -A 10 "await rds.waitFor" indexer/services/roundtable/src/helpers/aws.tsLength of output: 494
Script:
#!/bin/bash # Display the lines following the describeDBSnapshots call to see the catch block for error handling rg -A 10 "const statusResponse = await rds.describeDBSnapshots" indexer/services/roundtable/src/helpers/aws.tsLength of output: 511
indexer/services/roundtable/src/index.ts (2)
11-23: The import statements for the new tasks
deleteOldFastSyncSnapshotsandtakeFastSyncSnapshotTaskare correct and follow the existing pattern.105-119: The conditional loop starts for
takeFastSyncSnapshotTaskanddeleteOldFastSyncSnapshotsare implemented correctly, with appropriate interval settings from the configuration.indexer/services/roundtable/src/config.ts (3)
15-19: The addition of
FOUR_HOURS_IN_MILLISECONDSandONE_DAY_IN_MILLISECONDSconstants is correct and follows the existing pattern for time constants.70-81: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [45-78]
The new configuration flags for enabling the snapshot tasks and setting their intervals are correctly defined with sensible default values.
- 125-125: The
FAST_SYNC_SNAPSHOT_IDENTIFIER_PREFIXconstant is correctly set with a default value, ensuring snapshots can be identified consistently.
| /** | ||
| * Checks if the difference between two dates is less than a given interval. | ||
| * | ||
| * @param startDate | ||
| * @param endDate | ||
| * @param intervalMs | ||
| */ | ||
| function isDifferenceLessThanInterval( | ||
| startDate: string, | ||
| endDate: string, | ||
| intervalMs: number, | ||
| ): boolean { | ||
| const parseDateString = (dateStr: string): Date => { | ||
| const [year, month, day, hour, minute] = dateStr.split('-').map(Number); | ||
| return new Date(year, month, day, hour, minute); | ||
| }; | ||
|
|
||
| // Parse the date strings | ||
| const parsedDate1 = parseDateString(startDate); | ||
| const parsedDate2 = parseDateString(endDate); | ||
|
|
||
| // Calculate the difference in milliseconds | ||
| const differenceInMilliseconds = Math.abs(parsedDate1.getTime() - parsedDate2.getTime()); | ||
|
|
||
| // Compare with the interval | ||
| return differenceInMilliseconds < intervalMs; | ||
| } |
There was a problem hiding this comment.
Consider using a well-tested date parsing library like luxon for the isDifferenceLessThanInterval function to reduce the risk of date parsing errors.
- const parseDateString = (dateStr: string): Date => {
- const [year, month, day, hour, minute] = dateStr.split('-').map(Number);
- return new Date(year, month, day, hour, minute);
- };
+ const parseDateString = (dateStr: string): Date => DateTime.fromFormat(dateStr, 'yyyy-MM-dd-HH-mm').toJSDate();Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| /** | |
| * Checks if the difference between two dates is less than a given interval. | |
| * | |
| * @param startDate | |
| * @param endDate | |
| * @param intervalMs | |
| */ | |
| function isDifferenceLessThanInterval( | |
| startDate: string, | |
| endDate: string, | |
| intervalMs: number, | |
| ): boolean { | |
| const parseDateString = (dateStr: string): Date => { | |
| const [year, month, day, hour, minute] = dateStr.split('-').map(Number); | |
| return new Date(year, month, day, hour, minute); | |
| }; | |
| // Parse the date strings | |
| const parsedDate1 = parseDateString(startDate); | |
| const parsedDate2 = parseDateString(endDate); | |
| // Calculate the difference in milliseconds | |
| const differenceInMilliseconds = Math.abs(parsedDate1.getTime() - parsedDate2.getTime()); | |
| // Compare with the interval | |
| return differenceInMilliseconds < intervalMs; | |
| } | |
| /** | |
| * Checks if the difference between two dates is less than a given interval. | |
| * | |
| * @param startDate | |
| * @param endDate | |
| * @param intervalMs | |
| */ | |
| function isDifferenceLessThanInterval( | |
| startDate: string, | |
| endDate: string, | |
| intervalMs: number, | |
| ): boolean { | |
| const parseDateString = (dateStr: string): Date => DateTime.fromFormat(dateStr, 'yyyy-MM-dd-HH-mm').toJSDate(); | |
| // Parse the date strings | |
| const parsedDate1 = parseDateString(startDate); | |
| const parsedDate2 = parseDateString(endDate); | |
| // Calculate the difference in milliseconds | |
| const differenceInMilliseconds = Math.abs(parsedDate1.getTime() - parsedDate2.getTime()); | |
| // Compare with the interval | |
| return differenceInMilliseconds < intervalMs; | |
| } |
Changelist
add roundtable task to take fast sync Postgres snapshots every 4 hours
Test Plan
unit tested and tested in dev env
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.