add cluster matching sync timestamp and logs #1913
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces a new timestamp column Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
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:
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/services/cronJobs/syncEstimatedClusterMatchingJob.ts (1)
Line range hint
63-71: Remove duplicate Thread.terminate() call.The worker thread is being terminated twice - once in the
finallyblock and once after it. This could lead to attempting to terminate an already terminated thread.Remove the duplicate termination:
} catch (e) { logger.error('fetchAndUpdateClusterEstimatedMatching error', e); } finally { await Thread.terminate(matchingWorker); } - - await Thread.terminate(matchingWorker); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
migration/1737544105947-addClusterMatchingSyncAtTimestamp.ts(1 hunks)src/entities/qfRound.ts(1 hunks)src/services/cronJobs/syncEstimatedClusterMatchingJob.ts(1 hunks)src/workers/cocm/estimatedClusterMatchingWorker.ts(1 hunks)
🔇 Additional comments (3)
migration/1737544105947-addClusterMatchingSyncAtTimestamp.ts (1)
1-25: LGTM! Well-structured migration with proper safety checks.The migration follows TypeORM best practices with proper up/down methods and includes a safety check before adding the column.
src/services/cronJobs/syncEstimatedClusterMatchingJob.ts (1)
59-62: LGTM! Timestamp update is properly placed after successful sync.The timestamp update is correctly placed after the successful completion of the sync operation.
src/workers/cocm/estimatedClusterMatchingWorker.ts (1)
30-30: LGTM! Debug log added for operation tracking.The debug log for operation start is appropriately placed.
| logger.debug('fetchEstimatedClusterMatching() has been called'); | ||
| const matchingData = | ||
| await getClusterMatchingAdapter().fetchEstimatedClusterMatchings( | ||
| matchingDataInput, | ||
| ); | ||
| logger.debug( | ||
| 'fetchEstimatedClusterMatching() has worked with params', | ||
| String(matchingData), | ||
| ); | ||
| return matchingData; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve logging security for matchingData.
Converting matchingData directly to string could expose sensitive information in logs. Consider logging only necessary metadata.
Suggested improvement:
- logger.debug(
- 'fetchEstimatedClusterMatching() has worked with params',
- String(matchingData),
- );
+ logger.debug(
+ 'fetchEstimatedClusterMatching() completed',
+ {
+ recordCount: matchingData?.length || 0,
+ timestamp: new Date().toISOString()
+ }
+ );📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger.debug('fetchEstimatedClusterMatching() has been called'); | |
| const matchingData = | |
| await getClusterMatchingAdapter().fetchEstimatedClusterMatchings( | |
| matchingDataInput, | |
| ); | |
| logger.debug( | |
| 'fetchEstimatedClusterMatching() has worked with params', | |
| String(matchingData), | |
| ); | |
| return matchingData; | |
| logger.debug('fetchEstimatedClusterMatching() has been called'); | |
| const matchingData = | |
| await getClusterMatchingAdapter().fetchEstimatedClusterMatchings( | |
| matchingDataInput, | |
| ); | |
| logger.debug( | |
| 'fetchEstimatedClusterMatching() completed', | |
| { | |
| recordCount: matchingData?.length || 0, | |
| timestamp: new Date().toISOString() | |
| } | |
| ); | |
| return matchingData; |
src/entities/qfRound.ts
Outdated
| @Field(_type => Date) | ||
| @Column({ nullable: true }) | ||
| clusterMatchingSyncAt: Date; |
There was a problem hiding this comment.
Align GraphQL and database column nullability.
The field is nullable in the database but required in GraphQL. This mismatch could cause runtime errors when the field is null.
Update the field definition to match nullability:
- @Field(_type => Date)
+ @Field(_type => Date, { nullable: true })
@Column({ nullable: true })
clusterMatchingSyncAt: Date;📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Field(_type => Date) | |
| @Column({ nullable: true }) | |
| clusterMatchingSyncAt: Date; | |
| @Field(_type => Date, { nullable: true }) | |
| @Column({ nullable: true }) | |
| clusterMatchingSyncAt: Date; |
* throw error on un-vouching the givbackseligible projects * update message * optimize the approveMultipleProjects * make projects verified if they become givbacksEligible * prevent approve or reject draft projects * fix records * fix conditions * use redirectUrl * fix tests * add unverifyProjectsTestCases * add test:projectVerificationTab * temporary comment * fix typo * send email when project verification status changed * Feat/Generating public user data * added tests for querying user basic data * add includeUnlisted to FilterProjectQueryInputParams * return proper projects * add recipient address to streams when nonexistent (#1890) * started endaoment update feature * Superfluid Base Support (#1893) * finish project endpoint for superfluid to read * add filters for network and tokens for recurringdonations * fix verification logic and emails for base network streams * Update src/resolvers/recurringDonationResolver.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * comment network test and add cbBTC to seeds --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * remove project validation from anchor contract * Add networkId logic to superfluid subgraphs (#1896) * add networkId logic to superfluid subgraphs * remove networkId from api call to superfluid * fix eslint * fix linkedin scope * fix user info link to user info * cron job for sitemap generating * adding additional projects to Endaoment list * started cronjob * finished cron job * Feature cluster matching (#1862) * add cluster matching entity * add cluster matching adapters * finish cocm adapter * improve error handling for cluster matching * comment broken contract tests by missing eth_getCode Method * add feedback to handle qf cases * add cluster matching job to bootstrap file * fix coderabbit feedback PR * termine worker if an exception is raised * fix updateUser condition to handle email undefined case * fixed one variable; added cronjob env suggested by Carlos * removed redundant code * check config value * fix calling env variable * fix/removing endaomentId from update * add qfStrategy to qfRounds (#1903) * update bootstrap.js adding check endaoment * adding sitemap cronjob to bootstrap * additional logger data * fine tuninnig log * improve logger * fixing endaoment id * Set default zero for power balance snapshot on no return from balance aggregator (#1732) Ref #1655 * Fix/Sitemap env variables * fix missing prefix for url * fix matching cap calculation * fix data insertion for cluster matching * add user passport score null case to clustermatching queries * fix error handling in cocm adapter * add cluster matching sync timestamp and logs (#1913) * add cluster matching sync timestamp and logs * add nullability to clustermathicng syncAT * fix db call in worker for cluster matching * add uniquness constraint to estimatedclustedMatching * handle undefined case for instant power boosting services * better error handling in worker job * fixing prettier problem * Disable cluster matching --------- Co-authored-by: Cherik <[email protected]> Co-authored-by: kkatusic <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Mitch <[email protected]> Co-authored-by: Lovel George <[email protected]> Co-authored-by: Amin Latifi <[email protected]>
relate: Giveth/giveth-dapps-v2#4274 (comment)
Summary by CodeRabbit
Release Notes
clusterMatchingSyncAtcolumn in QF Round table to record sync timing.