-
Notifications
You must be signed in to change notification settings - Fork 92
Generate optimized SQL query to improve performance of find_chunks_by_dedup_key method #9318
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
3eaf530
9295259
612408d
8e11ad5
3be5ef6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -380,6 +380,42 @@ mocha.describe('md_store', function() { | |
| return md_store.delete_chunks_by_ids(_.map(chunks, '_id')); | ||
| }); | ||
|
|
||
| mocha.it('find_chunks_by_dedup_key()', async () => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. worth adding more tests for different cases. e.g.: test that we return an empty array when there is no match, etc.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added one test for this scenario. |
||
| if (config.DB_TYPE !== 'postgres') return; // feature uses SQL path | ||
| const bucket = { _id: md_store.make_md_id(), system: { _id: system_id } }; | ||
| const chunk = { | ||
| _id: md_store.make_md_id(), | ||
| system: system_id, | ||
| bucket: bucket._id, | ||
| frags: [{ _id: md_store.make_md_id() }], | ||
| size: 10, | ||
| frag_size: 10, | ||
| dedup_key: Buffer.from('noobaa') | ||
| }; | ||
| await md_store.insert_chunks([chunk]); | ||
| const chunksArr = await md_store.find_chunks_by_dedup_key(bucket, [Buffer.from('noobaa').toString('base64')]); | ||
| assert(Array.isArray(chunksArr)); | ||
| assert(chunksArr.length >= 1); | ||
| assert(chunksArr[0].frags[0]?._id?.toString() === chunk.frags[0]._id.toString()); | ||
| }); | ||
|
|
||
| mocha.it('test find_chunks_by_dedup_key - dedup_key doesnt exist in DB', async () => { | ||
| if (config.DB_TYPE !== 'postgres') return; // feature uses SQL path | ||
| const bucket = { _id: md_store.make_md_id(), system: { _id: system_id } }; | ||
| const chunksArr = await md_store.find_chunks_by_dedup_key(bucket, [Buffer.from('unknownkey').toString('base64')]); | ||
| assert(Array.isArray(chunksArr)); | ||
| assert(chunksArr.length === 0); | ||
| }); | ||
|
|
||
| mocha.it('find_chunks_by_dedup_key empty dedup_key array passed', async () => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does this specific case need to be tested? We don't expect an empty array, IIRC
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We return early from the function in case an empty dedup_keys array is passed. So, added this test case. |
||
| if (config.DB_TYPE !== 'postgres') return; // feature uses SQL path | ||
| const bucket = { _id: md_store.make_md_id(), system: { _id: system_id } }; | ||
|
|
||
| const chunksArr = await md_store.find_chunks_by_dedup_key(bucket, []); | ||
| assert(Array.isArray(chunksArr)); | ||
| assert(chunksArr.length === 0); | ||
| }); | ||
|
|
||
| }); | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -113,6 +113,10 @@ class ReclaimerMock extends AgentBlocksReclaimer { | |
| } | ||
|
|
||
|
|
||
| // @ts-ignore | ||
| // Do not run below tests if DB is not PostgreSQL | ||
| if (config.DB_TYPE !== 'postgres') return; | ||
|
Comment on lines
+116
to
+118
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move the PostgreSQL guard inside the test suite for consistency and correctness. The module-level 🔎 Proposed fix-// @ts-ignore
-// Do not run below tests if DB is not PostgreSQL
-if (config.DB_TYPE !== 'postgres') return;
-
mocha.describe('not mocked agent_blocks_reclaimer', function() {
+ // Do not run below tests if DB is not PostgreSQL
+ if (config.DB_TYPE !== 'postgres') return;
const object_io = new ObjectIO();🧰 Tools🪛 Biome (2.1.2)[error] 118-118: Illegal return statement outside of a function (parse) 🤖 Prompt for AI Agents |
||
|
|
||
| mocha.describe('not mocked agent_blocks_reclaimer', function() { | ||
|
|
||
| const object_io = new ObjectIO(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,10 @@ const key_rotator = new KeyRotator({ name: 'kr'}); | |
|
|
||
| config.MIN_CHUNK_AGE_FOR_DEDUP = 0; | ||
|
|
||
| // @ts-ignore | ||
| // Do not run below tests if DB is not PostgreSQL | ||
| if (config.DB_TYPE !== 'postgres') return; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @karthikps97 @liranmauda, why is this needed?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These tests are executed as part of both PostgreSQL tests and MongoDB tests. We are skipping this for the MongoDB run as it will fail since executeSQL is not implemented by mongo client. |
||
|
|
||
|
Comment on lines
+34
to
+37
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix illegal top‑level The guard: // @ts-ignore
// Do not run below tests if DB is not PostgreSQL
if (config.DB_TYPE !== 'postgres') return;is at file scope, which is a syntax error in Node/JS ( Suggest moving the DB-type guard into Mocha context and removing the config.MIN_CHUNK_AGE_FOR_DEDUP = 0;
-
-// @ts-ignore
-// Do not run below tests if DB is not PostgreSQL
-if (config.DB_TYPE !== 'postgres') return;
+// Run these tests only when DB is PostgreSQL
+const IS_POSTGRES = config.DB_TYPE === 'postgres';
@@
-mocha.describe('Encryption tests', function() {
+mocha.describe('Encryption tests', function() {
+ mocha.before(function() {
+ if (!IS_POSTGRES) this.skip();
+ });
@@
-mocha.describe('Rotation tests', function() {
+mocha.describe('Rotation tests', function() {
+ mocha.before(function() {
+ if (!IS_POSTGRES) this.skip();
+ });This keeps the suite PostgreSQL-only, avoids top-level control flow, and gives Mocha proper “skipped” semantics instead of a parse error. You can then drop the 🧰 Tools🪛 Biome (2.1.2)[error] 36-36: Illegal return statement outside of a function (parse) 🤖 Prompt for AI Agents
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was a psedo-code On any top level describe. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @liranmauda, thanks for the clarification! I understand now—you want to use a Here's how to apply this pattern to both top-level describes in the file: config.MIN_CHUNK_AGE_FOR_DEDUP = 0;
mocha.describe('Encryption tests', function() {
mocha.before(function() {
if (config.DB_TYPE !== 'postgres') {
this.skip();
}
});
const { rpc_client, EMAIL, SYSTEM } = coretest;
let response_account;
// ... rest of the tests
});
// ... (other code)
mocha.describe('Rotation tests', function() {
mocha.before(function() {
if (config.DB_TYPE !== 'postgres') {
this.skip();
}
});
const { rpc_client, EMAIL, SYSTEM } = coretest;
let accounts;
let buckets;
// ... rest of the tests
});This removes the illegal top-level
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Skipping the test using before hook would still run all the "after" hooks. This would throw errors specifically in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| mocha.describe('Encryption tests', function() { | ||
| const { rpc_client, EMAIL, SYSTEM } = coretest; | ||
| let response_account; | ||
|
|
@@ -131,7 +135,7 @@ mocha.describe('Encryption tests', function() { | |
| })); | ||
| }); | ||
|
|
||
| mocha.it('create accounts and compare acount access keys succefully', async function() { | ||
| mocha.it('create accounts and compare acount access keys succefully', async function() { | ||
| this.timeout(600000); // eslint-disable-line no-invalid-this | ||
| const db_system = await db_client.collection('systems').findOne({ name: SYSTEM }); | ||
| const new_account_params = { | ||
|
|
@@ -141,7 +145,7 @@ mocha.describe('Encryption tests', function() { | |
| let i; | ||
| for (i = 0; i < 20; i++) { | ||
| response_account = await rpc_client.account.create_account({...new_account_params, | ||
| email: `email${i}`, name: `name${i}`}); | ||
| email: `email${i}`, name: `name${i}`}); | ||
| accounts.push({email: `email${i}`, create_account_result: response_account, index: i}); | ||
| const db_account = await db_client.collection('accounts').findOne({ email: `email${i}` }); | ||
| const system_store_account = account_by_name(system_store.data.accounts, `email${i}`); | ||
|
|
@@ -213,7 +217,7 @@ mocha.describe('Encryption tests', function() { | |
| const system_store_account = account_by_name(system_store.data.accounts, cur_account.email); | ||
| const db_ns_resource = await db_client.collection('namespace_resources').findOne({ name: namespace_resource_name }); | ||
| const system_store_ns_resource = pool_by_name(system_store.data.namespace_resources, | ||
| namespace_resource_name); // system store data supposed to be decrypted | ||
| namespace_resource_name); // system store data supposed to be decrypted | ||
| // check s3 creds key in db is encrypted | ||
| const secrets = { | ||
| db_secret: db_ns_resource.connection.secret_key, | ||
|
|
@@ -448,7 +452,7 @@ mocha.describe('Encryption tests', function() { | |
| name: BKT, | ||
| }); | ||
| }); | ||
| mocha.it('regenerate creds for all accounts (non coretest) succefully', async function() { | ||
| mocha.it('regenerate creds for all accounts (non coretest) succefully', async function() { | ||
| this.timeout(600000); // eslint-disable-line no-invalid-this | ||
| await P.all(_.map(accounts, async cur_account => { | ||
| await rpc_client.account.generate_account_keys({ email: cur_account.email }); | ||
|
|
@@ -466,7 +470,7 @@ mocha.describe('Encryption tests', function() { | |
| })); | ||
| }); | ||
| // TODO: remove the comment | ||
| mocha.it('delete accounts succefully', async function() { | ||
| mocha.it('delete accounts succefully', async function() { | ||
| this.timeout(600000); // eslint-disable-line no-invalid-this | ||
| await P.all(_.map(accounts, async cur_account => { | ||
| await rpc_client.account.delete_account({ email: cur_account.email}); | ||
|
|
@@ -479,7 +483,6 @@ mocha.describe('Encryption tests', function() { | |
| }); | ||
| }); | ||
|
|
||
|
|
||
| /////////////// ROTATION & DISABLE & ENABLE TESTS ///////////////////////// | ||
| mocha.describe('Rotation tests', function() { | ||
| const { rpc_client, EMAIL, SYSTEM } = coretest; | ||
|
|
@@ -588,7 +591,7 @@ mocha.describe('Rotation tests', function() { | |
| await system_store.load(); | ||
| const original_secrets = await get_account_secrets_from_system_store_and_db(accounts[0].email, 's3_creds'); | ||
| await rpc_client.system.disable_master_key({entity: new SensitiveString(accounts[0].email), | ||
| entity_type: 'ACCOUNT'}); | ||
| entity_type: 'ACCOUNT'}); | ||
| await system_store.load(); | ||
| const system_store_account = account_by_name(system_store.data.accounts, accounts[0].email); | ||
| const secrets = await get_account_secrets_from_system_store_and_db(accounts[0].email, 's3_creds'); | ||
|
|
@@ -611,7 +614,7 @@ mocha.describe('Rotation tests', function() { | |
| await system_store.load(); | ||
| const original_secrets = await get_account_secrets_from_system_store_and_db(accounts[2].email, 's3_creds'); | ||
| await rpc_client.system.disable_master_key({entity: new SensitiveString(accounts[2].email), | ||
| entity_type: 'ACCOUNT'}); | ||
| entity_type: 'ACCOUNT'}); | ||
| await system_store.load(); | ||
| const system_store_account = account_by_name(system_store.data.accounts, accounts[2].email); | ||
| const secrets = await get_account_secrets_from_system_store_and_db(accounts[2].email, 's3_creds'); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move the PostgreSQL guard inside the test suite for consistency and correctness.
The module-level
returnstatement is technically invalid JavaScript syntax (flagged by static analysis) and creates inconsistency across the codebase. Other test files in this PR place the guard inside the test suite function (e.g.,test_tiering_ttl_worker.jsat line 67,test_object_io.jsat line 45), which is both syntactically correct and follows mocha best practices.🔎 Proposed fix
📝 Committable suggestion
🧰 Tools
🪛 Biome (2.1.2)
[error] 180-180: Illegal return statement outside of a function
(parse)
🤖 Prompt for AI Agents