Skip to content

Bugfix/job generator db connections lfx 1830 #2709

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Dec 4, 2024
Merged

Bugfix/job generator db connections lfx 1830 #2709

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a7ba334
small fix
themarolt Dec 3, 2024
3247a84
fix
themarolt Dec 4, 2024
ae78058
Merge branch 'main' into bugfix/job-generator-db-connections-LFX-1830
themarolt Dec 4, 2024
6c9151c
Merge branch 'main' into bugfix/job-generator-db-connections-LFX-1830
themarolt Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions backend/src/bin/jobs/refreshMaterializedViews.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { QueryTypes } from 'sequelize'

import { Logger, getChildLogger, getServiceChildLogger, logExecutionTimeV2 } from '@crowd/logging'

import { databaseInit } from '@/database/databaseConnection'
import { databaseClose, databaseInit } from '@/database/databaseConnection'

import { CrowdJob } from '../../types/jobTypes'

Expand Down Expand Up @@ -63,8 +63,14 @@ const job: CrowdJob = {
onTrigger: async () => {
const database = await databaseInit(1000 * 60 * 15, true)
const log = getServiceChildLogger('RefreshMVJob')
await refreshMaterializedView('member_segments_mv', database, log)
await refreshMaterializedView('organization_segments_mv', database, log)
try {
await refreshMaterializedView('member_segments_mv', database, log)
await refreshMaterializedView('organization_segments_mv', database, log)
} catch (err) {
log.error(err, 'Error while refreshing materialized views!')
} finally {
await databaseClose(database)
}
Comment on lines +66 to +73
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve error handling granularity and connection timeout

While the resource cleanup is good, consider these improvements:

  1. The 15-minute database timeout might mask connection issues
  2. Error handling could be more specific to different failure scenarios

Consider this implementation:

  const database = await databaseInit(1000 * 60 * 5, true)  // Reduce to 5 minutes
  const log = getServiceChildLogger('RefreshMVJob')
  try {
-   await refreshMaterializedView('member_segments_mv', database, log)
-   await refreshMaterializedView('organization_segments_mv', database, log)
+   const results = await Promise.allSettled([
+     refreshMaterializedView('member_segments_mv', database, log),
+     refreshMaterializedView('organization_segments_mv', database, log)
+   ]);
+   results.forEach((result, index) => {
+     if (result.status === 'rejected') {
+       log.error(result.reason, `Failed to refresh view ${index}`);
+     }
+   });
  } catch (err) {
    log.error(err, 'Error while refreshing materialized views!')
  } finally {
    await databaseClose(database)
  }

Committable suggestion skipped: line range outside the PR's diff.

},
}

Expand Down
9 changes: 6 additions & 3 deletions backend/src/database/databaseConnection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,9 @@ export async function databaseInit(
queryTimeoutMilliseconds: number = 60000,
forceNewInstance: boolean = false,
databaseHostnameOverride: string = null,
profileQueries: boolean = false,
) {
if (profileQueries || forceNewInstance) {
return models(queryTimeoutMilliseconds, databaseHostnameOverride, profileQueries)
if (forceNewInstance) {
return models(queryTimeoutMilliseconds, databaseHostnameOverride)
}

if (!cached) {
Expand All @@ -21,3 +20,7 @@ export async function databaseInit(

return cached
}

export async function databaseClose(database) {
await database.sequelize.close()
}
Comment on lines +24 to +26
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Enhance databaseClose with type safety and error handling

The databaseClose function lacks type safety and error handling:

Consider this implementation:

- export async function databaseClose(database) {
+ interface Database {
+   sequelize: {
+     close(): Promise<void>;
+   };
+ }
+ export async function databaseClose(database: Database) {
+   if (!database?.sequelize) {
+     throw new Error('Invalid database instance provided');
+   }
    try {
      await database.sequelize.close();
+   } catch (error) {
+     throw new Error(`Failed to close database connection: ${error.message}`);
+   }
  }
📝 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.

Suggested change
export async function databaseClose(database) {
await database.sequelize.close()
}
interface Database {
sequelize: {
close(): Promise<void>;
};
}
export async function databaseClose(database: Database) {
if (!database?.sequelize) {
throw new Error('Invalid database instance provided');
}
try {
await database.sequelize.close();
} catch (error) {
throw new Error(`Failed to close database connection: ${error.message}`);
}
}

34 changes: 15 additions & 19 deletions backend/src/database/models/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { IS_CLOUD_ENV } from '@crowd/common'
* This module creates the Sequelize to the database and
* exports all the models.
*/
import { getServiceChildLogger, logExecutionTimeV2 } from '@crowd/logging'
import { getServiceChildLogger } from '@crowd/logging'

import { DB_CONFIG, SERVICE } from '../../conf'
import * as configTypes from '../../conf/configTypes'
Expand Down Expand Up @@ -47,11 +47,7 @@ function getCredentials(): Credentials {
}
}

async function models(
queryTimeoutMilliseconds: number,
databaseHostnameOverride = null,
profileQueries = false,
) {
async function models(queryTimeoutMilliseconds: number, databaseHostnameOverride = null) {
log.info('Initializing sequelize database connection!')
const database = {} as any

Expand Down Expand Up @@ -106,19 +102,19 @@ async function models(
},
)

if (profileQueries) {
const oldQuery = sequelize.query
sequelize.query = async (query, options) => {
const { replacements } = options || {}
const result = await logExecutionTimeV2(
() => oldQuery.apply(sequelize, [query, options]),
log,
`DB Query:\n${query}\n${replacements ? `Params: ${JSON.stringify(replacements)}` : ''}`,
)

return result
}
}
// if (profileQueries) {
// const oldQuery = sequelize.query
// sequelize.query = async (query, options) => {
// const { replacements } = options || {}
// const result = await logExecutionTimeV2(
// () => oldQuery.apply(sequelize, [query, options]),
// log,
// `DB Query:\n${query}\n${replacements ? `Params: ${JSON.stringify(replacements)}` : ''}`,
// )

// return result
// }
// }
Comment on lines +105 to +117
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Remove commented out code

Instead of keeping commented out code in the codebase, it should be removed as it's preserved in version control history. This improves code maintainability and reduces confusion.

-  // if (profileQueries) {
-  //   const oldQuery = sequelize.query
-  //   sequelize.query = async (query, options) => {
-  //     const { replacements } = options || {}
-  //     const result = await logExecutionTimeV2(
-  //       () => oldQuery.apply(sequelize, [query, options]),
-  //       log,
-  //       `DB Query:\n${query}\n${replacements ? `Params: ${JSON.stringify(replacements)}` : ''}`,
-  //     )
-
-  //     return result
-  //   }
-  // }


const modelClasses = [
require('./auditLog').default,
Expand Down
5 changes: 2 additions & 3 deletions backend/src/middlewares/databaseMiddleware.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,10 @@ const log = getServiceLogger()
let qdb
export async function databaseMiddleware(req, res, next) {
try {
const profileQueries = !!req.profileSql
const database = await databaseInit(undefined, undefined, undefined, profileQueries)
const database = await databaseInit()
req.database = database
if (!qdb) {
qdb = await getClientSQL(profileQueries)
qdb = await getClientSQL()
}
req.qdb = qdb
} catch (error) {
Expand Down
9 changes: 0 additions & 9 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading