NSFS | Improve list objects performance on top of NS FS (PR 2/3) #8422
Conversation
naveenpaul1
force-pushed
the
new_nsfs_list_objects_2
branch
from
2c43a85 to
4c41484
Compare
naveenpaul1
force-pushed
the
new_nsfs_list_objects_2
branch
from
63851b5 to
2fdead0
Compare
naveenpaul1
force-pushed
the
new_nsfs_list_objects_2
branch
4 times, most recently
from
51cec93 to
77fc48b
Compare
naveenpaul1
force-pushed
the
new_nsfs_list_objects_2
branch
from
8034ed2 to
6b417cd
Compare
naveenpaul1
force-pushed
the
new_nsfs_list_objects_2
branch
4 times, most recently
from
b2f5de0 to
de2f83b
Compare
naveenpaul1
force-pushed
the
new_nsfs_list_objects_2
branch
from
de2f83b to
c973335
Compare
Signed-off-by: naveenpaul1 <[email protected]>
naveenpaul1
force-pushed
the
new_nsfs_list_objects_2
branch
from
c973335 to
d8b024e
Compare
|
This PR had no activity for too long - it will now be labeled stale. Update it to prevent it from getting closed. |
|
This PR had no activity for too long - it will now be labeled stale. Update it to prevent it from getting closed. |
WalkthroughA new unsorted listing mode for NamespaceFS object listings has been introduced, controlled by a configuration flag and an API parameter. This includes a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant S3_Endpoint
participant NamespaceFS
participant KeyMarkerFS
Client->>API: list_objects({list_type})
API->>S3_Endpoint: list_objects({list_type})
S3_Endpoint->>NamespaceFS: _list_objects(params, object_sdk, list_versions)
NamespaceFS->>KeyMarkerFS: (if list_type == "2") parse marker, manage positions
NamespaceFS-->>NamespaceFS: Perform sorted or unsorted listing
NamespaceFS->>NamespaceFS: prepare_result(..., list_type)
NamespaceFS->>S3_Endpoint: results
S3_Endpoint->>API: results
API->>Client: results
Estimated code review effort4 (~90 minutes) Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 13
🔭 Outside diff range comments (1)
src/sdk/namespace_fs.js (1)
712-777: Refactor to reduce code duplication between insertion functionsThe two insertion functions share significant logic. Consider extracting common functionality to reduce duplication and improve maintainability.
+const should_skip_entry = (keymarker, r) => keymarker.key_marker_value === r.key; + +const handle_common_prefix = async (r, delimiter, process_dir, keymarker) => { + if (!delimiter && r.common_prefix) { + if (r.marker_pos && !keymarker.pre_dir.includes(r.pre_dir)) { + keymarker.add_previour_dir(r.pre_dir, r.marker_pos); + } + await process_dir(r.key); + return true; + } + return false; +}; const insert_entry_to_results_arr = async r => { + if (should_skip_entry(keymarker, r)) return; + let pos; // Since versions are arranged next to latest object in the latest first order, // no need to find the sorted last index. Push the ".versions/#VERSION_OBJECT" as // they are in order if (results.length && r.key < results[results.length - 1].key && !this._is_hidden_version_path(r.key)) { pos = _.sortedLastIndexBy(results, r, a => a.key); } else { pos = results.length; } if (pos >= limit) { is_truncated = true; return; // not added } - if (!delimiter && r.common_prefix) { - await process_dir(r.key); - } else { - if (keymarker.key_marker_value === r.key) { - return; - } + if (!(await handle_common_prefix(r, delimiter, process_dir, keymarker))) { if (pos < results.length) { results.splice(pos, 0, r); } else { results.push(r); } if (results.length > limit) { results.length = limit; is_truncated = true; } } };
🧹 Nitpick comments (12)
src/sdk/keymarker_fs.js (1)
2-2: Remove redundant 'use strict' directive.ES modules and CommonJS modules in Node.js are automatically in strict mode. This directive is unnecessary.
-'use strict';src/test/unit_tests/jest_tests/test_unsort_list_object.test.js (7)
3-3: Remove redundant 'use strict' directive.As with the previous file, this directive is unnecessary in module context.
-'use strict';
124-124: Fix typo in comment.-// reak first read after 3 entries. +// break first read after 3 entries.
158-158: Fix typo in comment.-// reak first read after 3 entries. +// break first read after 3 entries.
198-198: Fix typo in comment.-// Seak first read after 3 entries. +// Break first read after 500 entries.
223-224: Fix incorrect comment.The comment mentions 5 items but the test expects 10000.
-//total number of dir and files inside list_dir_root is 5 +//total number of dir and files inside list_dir_root is 10000 expect(total_dir_entries).toBe(10000);
490-490: Fix typo in function name.-async function validat_pagination(r, total_items) { +async function validate_pagination(r, total_items) {Remember to update all calls to this function as well.
494-494: Replace magic number with a named constant.The hardcoded value 1584 should be explained or defined as a constant for clarity.
+const EXPECTED_TOTAL_ITEMS = 1584; // Total number of test objects created + async function validate_pagination(r, total_items) { if (r.next_marker) { expect(r.is_truncated).toStrictEqual(true); } else { - expect(total_items).toEqual(1584); + expect(total_items).toEqual(EXPECTED_TOTAL_ITEMS); expect(r.is_truncated).toStrictEqual(false); } }src/sdk/namespace_fs.js (4)
652-663: Simplify keymarker initialization logicThe current implementation checks if
key_markeris an object, but this could be more explicit about the expected structure. Consider validating the object structure more thoroughly.let keymarker; -if (typeof(key_marker) === 'object') { +if (key_marker && typeof key_marker === 'object' && 'marker' in key_marker) { keymarker = new KeyMarkerFS(key_marker, true); } else { keymarker = new KeyMarkerFS({ marker: key_marker, marker_pos: '', pre_dir: [], pre_dir_pos: [], }); }
846-848: Add clarifying comment for skip_list logicThe purpose of the
skip_listflag isn't immediately clear. Add a comment to explain why items need to be skipped when returning to the bucket root.// Skip item listing in bucket root path when list flow coming from sub dir to bucket root path, // if do not skip items in bucket root path will list two times, // first in normal flow, and second when return back from sub dir. +// This flag prevents duplicate listings of root directory items that were already +// processed before descending into subdirectories. if (this.bucket_path + '/' === dir_path) { skip_list = true; }
1049-1061: Consider type safety for list_type comparisonThe code compares
list_type === '2'as a string, but there's no validation that list_type is actually a string. This could lead to unexpected behavior if a number is passed.-res.next_marker = list_type === '2' ? { +res.next_marker = String(list_type) === '2' ? { marker: _get_filename(next_version_id_marker), marker_pos: r.marker_pos ? r.marker_pos.toString() : '', pre_dir: keymarker.pre_dir, pre_dir_pos: keymarker.pre_dir_pos, } : _get_filename(next_version_id_marker);
826-830: Handle unsorted listing request when feature is disabledWhen
list_type === '2'is requested butNSFS_LIST_OBJECTS_V2_UNSORTED_ENABLEDis false, the code silently falls back to sorted listing. Consider logging a warning or throwing an error to make this behavior explicit.if (config.NSFS_LIST_OBJECTS_V2_UNSORTED_ENABLED && list_type === '2') { await insert_unsort_entry_to_results_arr(r); } else { + if (list_type === '2' && !config.NSFS_LIST_OBJECTS_V2_UNSORTED_ENABLED) { + dbg.warn('Unsorted listing requested but feature is disabled, falling back to sorted listing'); + } await insert_entry_to_results_arr(r); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
config.js(1 hunks)src/api/object_api.js(2 hunks)src/endpoint/s3/ops/s3_get_bucket.js(1 hunks)src/native/fs/fs_napi.cpp(1 hunks)src/sdk/keymarker_fs.js(1 hunks)src/sdk/namespace_fs.js(14 hunks)src/test/system_tests/test_utils.js(2 hunks)src/test/unit_tests/jest_tests/test_list_object.test.js(0 hunks)src/test/unit_tests/jest_tests/test_unsort_list_object.test.js(1 hunks)src/test/unit_tests/test_namespace_fs.js(2 hunks)src/test/unit_tests/test_namespace_fs_mpu.js(1 hunks)
🧠 Learnings (1)
src/test/unit_tests/test_namespace_fs.js (1)
Learnt from: naveenpaul1
PR: #9042
File: src/util/cloud_utils.js:183-194
Timestamp: 2025-06-17T12:59:51.543Z
Learning: In the set_noobaa_s3_connection function in src/util/cloud_utils.js, internal NooBaa S3 endpoints (filtered by 'api': 's3', 'kind': 'INTERNAL') will always be non-secure, so tls: false is the correct setting despite the conditional https:// scheme logic.
🧬 Code Graph Analysis (2)
src/test/unit_tests/jest_tests/test_unsort_list_object.test.js (5)
config.js (4)
fs(10-10)require(17-17)path(11-11)config(7-7)src/test/system_tests/test_utils.js (10)
fs(4-4)require(11-11)require(12-12)require(16-16)require(18-18)path(6-6)fs_utils(14-14)nb_native(15-15)config(10-10)TMP_PATH(22-22)src/test/unit_tests/test_namespace_fs_mpu.js (15)
fs(6-6)require(20-20)path(8-8)fs_utils(15-15)NamespaceFS(17-17)buffer_utils(19-19)crypto(11-11)config(14-14)upload_bkt(34-34)src_bkt(28-28)ns_tmp_bucket_path(30-30)i(65-65)i(493-493)ns_tmp(37-45)tmp_fs_path(29-29)src/sdk/namespace_fs.js (10)
fs(7-7)require(26-26)require(28-28)require(29-29)path(8-8)nb_native(24-24)buffer_utils(19-19)crypto(15-15)config(14-14)r(213-213)src/test/unit_tests/test_namespace_fs.js (37)
fs(7-7)require(22-22)require(24-24)require(25-25)path(9-9)fs_utils(17-17)nb_native(18-18)NamespaceFS(19-19)buffer_utils(21-21)crypto(11-11)config(15-15)DEFAULT_FS_CONFIG(40-40)upload_bkt(47-47)upload_bkt(717-717)upload_bkt(1363-1363)src_bkt(46-46)src_bkt(716-716)src_bkt(1362-1362)ns_tmp_bucket_path(54-54)ns_tmp_bucket_path(733-733)ns_tmp_bucket_path(1368-1368)i(264-264)dummy_object_sdk(52-52)dummy_object_sdk(731-731)dummy_object_sdk(1234-1234)dummy_object_sdk(1367-1367)ns_tmp(65-73)ns_tmp(734-734)ns_tmp(1369-1369)tmp_fs_path(51-51)tmp_fs_path(727-727)tmp_fs_path(1228-1228)tmp_fs_path(1364-1364)file(1208-1208)key(1457-1457)key(1502-1502)key(1545-1545)
src/sdk/keymarker_fs.js (1)
src/sdk/namespace_fs.js (1)
KeyMarkerFS(30-30)
🪛 Biome (1.9.4)
src/test/unit_tests/jest_tests/test_unsort_list_object.test.js
[error] 2-3: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
src/sdk/keymarker_fs.js
[error] 1-2: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
💤 Files with no reviewable changes (1)
- src/test/unit_tests/jest_tests/test_list_object.test.js
🧰 Additional context used
🧠 Learnings (1)
src/test/unit_tests/test_namespace_fs.js (1)
Learnt from: naveenpaul1
PR: #9042
File: src/util/cloud_utils.js:183-194
Timestamp: 2025-06-17T12:59:51.543Z
Learning: In the set_noobaa_s3_connection function in src/util/cloud_utils.js, internal NooBaa S3 endpoints (filtered by 'api': 's3', 'kind': 'INTERNAL') will always be non-secure, so tls: false is the correct setting despite the conditional https:// scheme logic.
🧬 Code Graph Analysis (2)
src/test/unit_tests/jest_tests/test_unsort_list_object.test.js (5)
config.js (4)
fs(10-10)require(17-17)path(11-11)config(7-7)src/test/system_tests/test_utils.js (10)
fs(4-4)require(11-11)require(12-12)require(16-16)require(18-18)path(6-6)fs_utils(14-14)nb_native(15-15)config(10-10)TMP_PATH(22-22)src/test/unit_tests/test_namespace_fs_mpu.js (15)
fs(6-6)require(20-20)path(8-8)fs_utils(15-15)NamespaceFS(17-17)buffer_utils(19-19)crypto(11-11)config(14-14)upload_bkt(34-34)src_bkt(28-28)ns_tmp_bucket_path(30-30)i(65-65)i(493-493)ns_tmp(37-45)tmp_fs_path(29-29)src/sdk/namespace_fs.js (10)
fs(7-7)require(26-26)require(28-28)require(29-29)path(8-8)nb_native(24-24)buffer_utils(19-19)crypto(15-15)config(14-14)r(213-213)src/test/unit_tests/test_namespace_fs.js (37)
fs(7-7)require(22-22)require(24-24)require(25-25)path(9-9)fs_utils(17-17)nb_native(18-18)NamespaceFS(19-19)buffer_utils(21-21)crypto(11-11)config(15-15)DEFAULT_FS_CONFIG(40-40)upload_bkt(47-47)upload_bkt(717-717)upload_bkt(1363-1363)src_bkt(46-46)src_bkt(716-716)src_bkt(1362-1362)ns_tmp_bucket_path(54-54)ns_tmp_bucket_path(733-733)ns_tmp_bucket_path(1368-1368)i(264-264)dummy_object_sdk(52-52)dummy_object_sdk(731-731)dummy_object_sdk(1234-1234)dummy_object_sdk(1367-1367)ns_tmp(65-73)ns_tmp(734-734)ns_tmp(1369-1369)tmp_fs_path(51-51)tmp_fs_path(727-727)tmp_fs_path(1228-1228)tmp_fs_path(1364-1364)file(1208-1208)key(1457-1457)key(1502-1502)key(1545-1545)
src/sdk/keymarker_fs.js (1)
src/sdk/namespace_fs.js (1)
KeyMarkerFS(30-30)
🪛 Biome (1.9.4)
src/test/unit_tests/jest_tests/test_unsort_list_object.test.js
[error] 2-3: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
src/sdk/keymarker_fs.js
[error] 1-2: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
🔇 Additional comments (10)
config.js (1)
846-847: LGTM! Well-placed configuration flag with appropriate defaults.The new
NSFS_LIST_OBJECTS_V2_UNSORTED_ENABLEDconfiguration flag is properly placed in the NSFS section and follows the existing naming conventions. The default value offalseensures backward compatibility by keeping the existing sorted behavior as the default.src/test/system_tests/test_utils.js (2)
494-512: Excellent refactoring! Centralized dummy SDK creation reduces duplication.The new
make_dummy_object_sdkutility function properly centralizes the creation of dummy SDK objects that were previously duplicated across multiple test files. The implementation includes all necessary components:
- Proper
requesting_accountstructure with NSFS account config- AbortController for request cancellation handling
throw_if_abortedmethod for abort signal checkingThis promotes consistency and maintainability across the test suite.
781-781: Proper export of the new utility function.The new utility function is correctly exported for use by other test modules.
src/native/fs/fs_napi.cpp (1)
2108-2108: LGTM! Appropriate debug level adjustment.Changing from
DBG0toDBG1reduces the verbosity of this internal directory position logging, which is appropriate for detailed tracing of the new unsorted listing functionality.src/endpoint/s3/ops/s3_get_bucket.js (1)
44-44: LGTM! Essential parameter forwarding fix.Adding
list_typeto the params object ensures the backend receives the listing type parameter, which is required for the new unsorted listing functionality to work correctly.src/test/unit_tests/test_namespace_fs_mpu.js (1)
20-20: LGTM! Good refactoring to use shared utility.The change to use the shared
make_dummy_object_sdkfunction from test utilities improves code maintainability by centralizing dummy SDK creation logic and reducing duplication across test files.Also applies to: 26-26
src/test/unit_tests/test_namespace_fs.js (2)
24-24: Good refactoring to centralize test utilities.Moving
make_dummy_object_sdkto a shared test utility module improves code reusability and maintainability across test files.
76-76: Appropriate configuration for existing tests.Setting
NSFS_LIST_OBJECTS_V2_UNSORTED_ENABLEDto false ensures existing tests run with the default sorted listing behavior, maintaining test isolation from the new unsorted feature.src/sdk/namespace_fs.js (2)
30-30: LGTM! Clean import statementThe import of
KeyMarkerFSmodule follows the existing pattern and is appropriately placed with other SDK imports.
3189-3190: LGTM! Improved code formattingThe line break improves readability by keeping the line length manageable.
| list_type: { | ||
| type: 'string', | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation constraints for the list_type parameter.
The list_type parameter lacks validation constraints such as enum values or a description of valid values. This could lead to runtime errors or unexpected behavior if invalid values are passed.
Consider adding enum constraints to define valid list types:
list_type: {
type: 'string',
+ enum: ['sorted', 'unsorted'],
+ description: 'Specifies the listing mode: sorted (default) or unsorted'
},📝 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.
| list_type: { | |
| type: 'string', | |
| }, | |
| list_type: { | |
| type: 'string', | |
| enum: ['sorted', 'unsorted'], | |
| description: 'Specifies the listing mode: sorted (default) or unsorted' | |
| }, |
🤖 Prompt for AI Agents
In src/api/object_api.js around lines 726 to 728, the list_type parameter is
missing validation constraints. Add an enum property listing all valid string
values for list_type and include a description explaining what values are
acceptable. This will enforce valid inputs and prevent runtime errors or
unexpected behavior.
| list_type: { | ||
| type: 'string', | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation constraints for the list_type parameter.
Similar to the list_objects method, this parameter needs validation constraints to ensure only valid values are accepted.
Apply the same enum constraints as suggested for the list_objects method:
list_type: {
type: 'string',
+ enum: ['sorted', 'unsorted'],
+ description: 'Specifies the listing mode: sorted (default) or unsorted'
},📝 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.
| list_type: { | |
| type: 'string', | |
| }, | |
| list_type: { | |
| type: 'string', | |
| enum: ['sorted', 'unsorted'], | |
| description: 'Specifies the listing mode: sorted (default) or unsorted' | |
| }, |
🤖 Prompt for AI Agents
In src/api/object_api.js around lines 783 to 785, the list_type parameter lacks
validation constraints. Add enum validation constraints to list_type similar to
those used in the list_objects method to restrict accepted values to a
predefined set. This ensures only valid list_type values are accepted and
improves input validation.
| constructor({ marker, marker_pos, pre_dir, pre_dir_pos }, is_unsorted = false) { | ||
| this.marker = marker; | ||
| this.marker_pos = marker_pos.toString(); | ||
| this.pre_dir = pre_dir; | ||
| this.pre_dir_pos = pre_dir_pos; | ||
| this.key_marker_value = marker; | ||
| this.current_dir = ''; | ||
| this.is_unsorted = is_unsorted; | ||
| this.last_pre_dir = ''; | ||
| this.last_pre_dir_position = ''; | ||
| if (is_unsorted) { | ||
| this.current_dir = pre_dir.length > 0 && marker.includes('/') ? | ||
| marker.substring(0, marker.lastIndexOf('/') + 1) : ''; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add parameter validation to prevent runtime errors.
The constructor should validate input parameters, especially marker_pos which could be null/undefined and cause toString() to fail.
constructor({ marker, marker_pos, pre_dir, pre_dir_pos }, is_unsorted = false) {
+ if (marker_pos === null || marker_pos === undefined) {
+ throw new Error('marker_pos is required');
+ }
this.marker = marker;
this.marker_pos = marker_pos.toString();📝 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.
| constructor({ marker, marker_pos, pre_dir, pre_dir_pos }, is_unsorted = false) { | |
| this.marker = marker; | |
| this.marker_pos = marker_pos.toString(); | |
| this.pre_dir = pre_dir; | |
| this.pre_dir_pos = pre_dir_pos; | |
| this.key_marker_value = marker; | |
| this.current_dir = ''; | |
| this.is_unsorted = is_unsorted; | |
| this.last_pre_dir = ''; | |
| this.last_pre_dir_position = ''; | |
| if (is_unsorted) { | |
| this.current_dir = pre_dir.length > 0 && marker.includes('/') ? | |
| marker.substring(0, marker.lastIndexOf('/') + 1) : ''; | |
| } | |
| } | |
| constructor({ marker, marker_pos, pre_dir, pre_dir_pos }, is_unsorted = false) { | |
| if (marker_pos === null || marker_pos === undefined) { | |
| throw new Error('marker_pos is required'); | |
| } | |
| this.marker = marker; | |
| this.marker_pos = marker_pos.toString(); | |
| this.pre_dir = pre_dir; | |
| this.pre_dir_pos = pre_dir_pos; | |
| this.key_marker_value = marker; | |
| this.current_dir = ''; | |
| this.is_unsorted = is_unsorted; | |
| this.last_pre_dir = ''; | |
| this.last_pre_dir_position = ''; | |
| if (is_unsorted) { | |
| this.current_dir = pre_dir.length > 0 && marker.includes('/') ? | |
| marker.substring(0, marker.lastIndexOf('/') + 1) : ''; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/sdk/keymarker_fs.js around lines 5 to 19, the constructor uses
marker_pos.toString() without checking if marker_pos is null or undefined, which
can cause runtime errors. Add validation to ensure marker_pos is defined before
calling toString(), and handle cases where it is null or undefined by assigning
a default string value or throwing a clear error.
| async update_key_marker(marker, marker_pos) { | ||
| this.marker = marker; | ||
| this.marker_pos = marker_pos; | ||
| this.key_marker_value = marker; | ||
| } | ||
|
|
||
| async get_previour_dir_length() { | ||
| return this.pre_dir.length; | ||
| } | ||
|
|
||
| async get_previour_dir_info() { | ||
| return { | ||
| pre_dir_path: this.pre_dir.pop(), | ||
| pre_dir_position: this.pre_dir_pos.pop(), | ||
| }; | ||
| } | ||
|
|
||
| async add_previour_dir(pre_dir, pre_dir_pos) { | ||
| this.pre_dir.push(pre_dir); | ||
| this.pre_dir_pos.push(pre_dir_pos.toString()); | ||
| } | ||
|
|
||
| async update_last_previour_dir(last_pre_dir, last_pre_dir_position) { | ||
| this.last_pre_dir = last_pre_dir; | ||
| this.last_pre_dir_position = last_pre_dir_position; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary async keywords from synchronous methods.
None of these methods perform asynchronous operations. The async keyword is misleading and should be removed.
-async update_key_marker(marker, marker_pos) {
+update_key_marker(marker, marker_pos) {
this.marker = marker;
this.marker_pos = marker_pos;
this.key_marker_value = marker;
}Apply the same change to all other methods in this class.
📝 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.
| async update_key_marker(marker, marker_pos) { | |
| this.marker = marker; | |
| this.marker_pos = marker_pos; | |
| this.key_marker_value = marker; | |
| } | |
| async get_previour_dir_length() { | |
| return this.pre_dir.length; | |
| } | |
| async get_previour_dir_info() { | |
| return { | |
| pre_dir_path: this.pre_dir.pop(), | |
| pre_dir_position: this.pre_dir_pos.pop(), | |
| }; | |
| } | |
| async add_previour_dir(pre_dir, pre_dir_pos) { | |
| this.pre_dir.push(pre_dir); | |
| this.pre_dir_pos.push(pre_dir_pos.toString()); | |
| } | |
| async update_last_previour_dir(last_pre_dir, last_pre_dir_position) { | |
| this.last_pre_dir = last_pre_dir; | |
| this.last_pre_dir_position = last_pre_dir_position; | |
| } | |
| update_key_marker(marker, marker_pos) { | |
| this.marker = marker; | |
| this.marker_pos = marker_pos; | |
| this.key_marker_value = marker; | |
| } | |
| get_previour_dir_length() { | |
| return this.pre_dir.length; | |
| } | |
| get_previour_dir_info() { | |
| return { | |
| pre_dir_path: this.pre_dir.pop(), | |
| pre_dir_position: this.pre_dir_pos.pop(), | |
| }; | |
| } | |
| add_previour_dir(pre_dir, pre_dir_pos) { | |
| this.pre_dir.push(pre_dir); | |
| this.pre_dir_pos.push(pre_dir_pos.toString()); | |
| } | |
| update_last_previour_dir(last_pre_dir, last_pre_dir_position) { | |
| this.last_pre_dir = last_pre_dir; | |
| this.last_pre_dir_position = last_pre_dir_position; | |
| } |
🤖 Prompt for AI Agents
In src/sdk/keymarker_fs.js from lines 21 to 46, several methods are marked as
async but do not perform any asynchronous operations. Remove the async keyword
from all these synchronous methods, including update_key_marker,
get_previour_dir_length, get_previour_dir_info, add_previour_dir, and
update_last_previour_dir. Also, review the rest of the class and remove async
from any other synchronous methods to avoid misleading usage.
| async get_previour_dir_length() { | ||
| return this.pre_dir.length; | ||
| } |
There was a problem hiding this comment.
Fix typo in method name.
"previour" should be "previous" for consistency and correctness.
-async get_previour_dir_length() {
+async get_previous_dir_length() {
return this.pre_dir.length;
}📝 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.
| async get_previour_dir_length() { | |
| return this.pre_dir.length; | |
| } | |
| async get_previous_dir_length() { | |
| return this.pre_dir.length; | |
| } |
🤖 Prompt for AI Agents
In src/sdk/keymarker_fs.js around lines 27 to 29, the method name
get_previour_dir_length contains a typo. Rename the method to
get_previous_dir_length to correct the spelling and maintain consistency.
| @@ -593,6 +594,7 @@ class NamespaceFS { | |||
| * prefix?: string, | |||
| * delimiter?: string, | |||
| * key_marker?: string, | |||
| * list_type?: string, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add JSDoc documentation for the new list_type parameter
The new list_type parameter lacks documentation about its purpose and accepted values. Consider adding JSDoc comments to clarify its usage.
/**
* @typedef {{
* bucket: string,
* prefix?: string,
* delimiter?: string,
* key_marker?: string,
- * list_type?: string,
+ * list_type?: string, // '2' for unsorted listing, undefined/other for sorted listing
* limit?: number,
* }} ListParams
*/Also applies to: 617-617
🤖 Prompt for AI Agents
In src/sdk/namespace_fs.js at lines 597 and 617, the new parameter `list_type`
is missing JSDoc documentation. Add JSDoc comments describing the purpose of
`list_type` and the accepted values it can take to clarify its usage for future
maintainers and users.
| const config = require('../../../../config'); | ||
|
|
||
| const DEFAULT_FS_CONFIG = get_process_fs_context(); | ||
| config.NSFS_LIST_OBJECTS_V2_UNSORTED_ENABLED = true; |
There was a problem hiding this comment.
Move global config modification into test lifecycle hooks.
Modifying global configuration at the file level can affect other tests running in parallel. This should only be done within beforeAll/afterAll hooks.
-config.NSFS_LIST_OBJECTS_V2_UNSORTED_ENABLED = true;This is already being set in the beforeAll hooks, so this line should be removed.
📝 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.
| config.NSFS_LIST_OBJECTS_V2_UNSORTED_ENABLED = true; |
🤖 Prompt for AI Agents
In src/test/unit_tests/jest_tests/test_unsort_list_object.test.js at line 18,
remove the global configuration modification setting
config.NSFS_LIST_OBJECTS_V2_UNSORTED_ENABLED = true from the top-level code and
rely solely on the existing beforeAll and afterAll hooks to set and reset this
configuration to avoid side effects on other tests.
| for (let i = 0; i < total_files; i++) { | ||
| create_temp_file(list_dir_root, `test_${i}.json`, {test: test}); | ||
| } |
There was a problem hiding this comment.
Fix undefined variable reference.
The variable test is not defined, which will cause a ReferenceError.
for (let i = 0; i < total_files; i++) {
- create_temp_file(list_dir_root, `test_${i}.json`, {test: test});
+ create_temp_file(list_dir_root, `test_${i}.json`, {test: 'test'});
}📝 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.
| for (let i = 0; i < total_files; i++) { | |
| create_temp_file(list_dir_root, `test_${i}.json`, {test: test}); | |
| } | |
| for (let i = 0; i < total_files; i++) { | |
| create_temp_file(list_dir_root, `test_${i}.json`, {test: 'test'}); | |
| } |
🤖 Prompt for AI Agents
In src/test/unit_tests/jest_tests/test_unsort_list_object.test.js around lines
89 to 91, the variable `test` used as an argument in create_temp_file is not
defined, causing a ReferenceError. Define the variable `test` with the
appropriate value or replace it with the correct existing variable before using
it in the create_temp_file call to fix the issue.
| } catch (err) { | ||
| console.log("Error :", err); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in tests.
Catching and logging errors without failing the test can hide issues. Consider using Jest's error assertions or re-throwing the error.
} catch (err) {
- console.log("Error :", err);
+ throw err; // Let Jest handle the 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (err) { | |
| console.log("Error :", err); | |
| } | |
| } catch (err) { | |
| throw err; // Let Jest handle the error | |
| } |
🤖 Prompt for AI Agents
In src/test/unit_tests/jest_tests/test_unsort_list_object.test.js around lines
146 to 148, the catch block currently logs errors without failing the test,
which can hide test failures. Modify the catch block to either re-throw the
caught error or use Jest's expect assertions to properly fail the test when an
error occurs, ensuring test failures are visible and handled correctly.
|
|
||
| it('list 10000 dir files - telldir and seekdir', async () => { | ||
| for (let i = total_files; i < total_files + 9995; i++) { | ||
| create_temp_file(list_dir_root, `test_${i}.json`, {test: test}); |
There was a problem hiding this comment.
Fix undefined variable reference.
Same issue as above - test variable is not defined.
-create_temp_file(list_dir_root, `test_${i}.json`, {test: test});
+create_temp_file(list_dir_root, `test_${i}.json`, {test: 'test'});📝 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.
| create_temp_file(list_dir_root, `test_${i}.json`, {test: test}); | |
| create_temp_file(list_dir_root, `test_${i}.json`, {test: 'test'}); |
🤖 Prompt for AI Agents
In src/test/unit_tests/jest_tests/test_unsort_list_object.test.js at line 191,
the variable 'test' used in the call to create_temp_file is undefined. Define or
replace 'test' with the correct variable or value that should be passed as the
third argument to create_temp_file to fix the undefined variable reference.
Explain the changes
NB : New parameter
next_posadded to APIlist_objectsto pass the seek position. I tried to pass this along withnext_markeras a object but Ceph test failed when comparingnext_marker.Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
ALLOW_NSFS_UNSORTED_OBJECTS_LIST, Result should retun with unsorted list.closed PR : #7983
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores