Skip to content

NSFS | Improve list objects performance on top of NS FS (PR 2/3) #8422

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions config.js
Original file line number Diff line number Diff line change
Expand Up @@ -843,6 +843,9 @@ config.NSFS_GLACIER_MIGRATE_INTERVAL = 15 * 60 * 1000;
// of `manage_nsfs glacier restore`
config.NSFS_GLACIER_RESTORE_INTERVAL = 15 * 60 * 1000;

// enable/disable unsorted listing application level
config.NSFS_LIST_OBJECTS_V2_UNSORTED_ENABLED = false;

// NSFS_GLACIER_EXPIRY_RUN_TIME must be of the format hh:mm which specifies
// when NooBaa should allow running glacier expiry process
// NOTE: This will also be in the same timezone as specified in
Expand Down
6 changes: 6 additions & 0 deletions src/api/object_api.js
Original file line number Diff line number Diff line change
Expand Up @@ -723,6 +723,9 @@ module.exports = {
limit: {
type: 'integer'
},
list_type: {
type: 'string',
},
Comment on lines +726 to +728
Copy link

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
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.

}
},
reply: {
Expand Down Expand Up @@ -777,6 +780,9 @@ module.exports = {
limit: {
type: 'integer'
},
list_type: {
type: 'string',
},
Comment on lines +783 to +785
Copy link

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
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.

}
},
reply: {
Expand Down
1 change: 1 addition & 0 deletions src/endpoint/s3/ops/s3_get_bucket.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ async function get_bucket(req) {
bucket: req.params.bucket,
prefix: req.query.prefix,
delimiter: req.query.delimiter,
list_type: list_type,
limit: Math.min(max_keys_received, 1000),
key_marker: list_type === '2' ?
(s3_utils.cont_tok_to_key_marker(cont_tok) || start_after) : req.query.marker,
Expand Down
2 changes: 1 addition & 1 deletion src/native/fs/fs_napi.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2105,7 +2105,7 @@ struct TellDir : public FSWrapWorker<DirWrap>
}
virtual void OnOK()
{
DBG0("FS::Telldir::OnOK: " << DVAL(_wrap->_path) << DVAL(_tell_res));
DBG1("FS::Telldir::OnOK: " << DVAL(_wrap->_path) << DVAL(_tell_res));
Napi::Env env = Env();
auto res = Napi::BigInt::New(env, _tell_res);
_deferred.Resolve(res);
Expand Down
50 changes: 50 additions & 0 deletions src/sdk/keymarker_fs.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/* Copyright (C) 2020 NooBaa */
'use strict';

class KeyMarkerFS {
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) : '';
}
}
Comment on lines +5 to +19
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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;
}
Comment on lines +27 to +29
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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.


async get_previour_dir_info() {
return {
pre_dir_path: this.pre_dir.pop(),
pre_dir_position: this.pre_dir_pos.pop(),
};
}
Comment on lines +31 to +36
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix typo in method name and consider the mutating behavior.

The method name has a typo, and the pop() operations mutate the internal state. Consider documenting this behavior or providing a non-mutating alternative.

-async get_previour_dir_info() {
+async get_previous_dir_info() {
     return {
         pre_dir_path: this.pre_dir.pop(),
         pre_dir_position: this.pre_dir_pos.pop(),
     };
 }
📝 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
async get_previour_dir_info() {
return {
pre_dir_path: this.pre_dir.pop(),
pre_dir_position: this.pre_dir_pos.pop(),
};
}
async get_previous_dir_info() {
return {
pre_dir_path: this.pre_dir.pop(),
pre_dir_position: this.pre_dir_pos.pop(),
};
}
🤖 Prompt for AI Agents
In src/sdk/keymarker_fs.js around lines 31 to 36, fix the typo in the method
name from get_previour_dir_info to get_previous_dir_info. Also, since using
pop() mutates the internal arrays pre_dir and pre_dir_pos, either document this
side effect clearly in the method's comments or change the implementation to
return the last elements without removing them to avoid mutating the internal
state.


async add_previour_dir(pre_dir, pre_dir_pos) {
this.pre_dir.push(pre_dir);
this.pre_dir_pos.push(pre_dir_pos.toString());
}
Comment on lines +38 to +41
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix typo in method name.

Maintain consistency in method naming.

-async add_previour_dir(pre_dir, pre_dir_pos) {
+async add_previous_dir(pre_dir, pre_dir_pos) {
     this.pre_dir.push(pre_dir);
     this.pre_dir_pos.push(pre_dir_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.

Suggested change
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 add_previous_dir(pre_dir, pre_dir_pos) {
this.pre_dir.push(pre_dir);
this.pre_dir_pos.push(pre_dir_pos.toString());
}
🤖 Prompt for AI Agents
In src/sdk/keymarker_fs.js around lines 38 to 41, the method name
add_previour_dir contains a typo. Rename the method to add_previous_dir to fix
the spelling mistake and maintain consistent naming conventions throughout the
codebase.


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;
}
Comment on lines +21 to +46
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 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.

Suggested change
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.

Comment on lines +43 to +46
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix typo in method name.

-async update_last_previour_dir(last_pre_dir, last_pre_dir_position) {
+async update_last_previous_dir(last_pre_dir, last_pre_dir_position) {
     this.last_pre_dir = last_pre_dir;
     this.last_pre_dir_position = last_pre_dir_position;
 }
📝 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
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;
}
async update_last_previous_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 around lines 43 to 46, the method name
update_last_previour_dir contains a typo. Rename the method to
update_last_previous_dir to correct the spelling mistake.

}

// EXPORTS
module.exports = KeyMarkerFS;
319 changes: 229 additions & 90 deletions src/sdk/namespace_fs.js

Large diffs are not rendered by default.

22 changes: 22 additions & 0 deletions src/test/system_tests/test_utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -491,6 +491,26 @@ function get_new_buckets_path_by_test_env(new_buckets_full_path, new_buckets_dir
return is_nc_coretest ? path.join(new_buckets_full_path, new_buckets_dir) : new_buckets_dir;
}


/**
* common dummy SDK for testing
*/
function make_dummy_object_sdk() {
return {
requesting_account: {
force_md5_etag: false,
nsfs_account_config: {
uid: process.getuid(),
gid: process.getgid(),
}
},
abort_controller: new AbortController(),
throw_if_aborted() {
if (this.abort_controller.signal.aborted) throw new Error('request aborted signal');
}
};
}

/**
* write_manual_config_file writes config file directly to the file system without using config FS
* used for creating backward compatibility tests, invalid config files etc
Expand Down Expand Up @@ -758,10 +778,12 @@ exports.create_identity_dir_if_missing = create_identity_dir_if_missing;
exports.symlink_account_name = symlink_account_name;
exports.symlink_account_access_keys = symlink_account_access_keys;
exports.create_file = create_file;
exports.make_dummy_object_sdk = make_dummy_object_sdk;
exports.create_redirect_file = create_redirect_file;
exports.delete_redirect_file = delete_redirect_file;
exports.create_system_json = create_system_json;
exports.update_system_json = update_system_json;
exports.fail_test_if_default_config_dir_exists = fail_test_if_default_config_dir_exists;
exports.create_config_dir = create_config_dir;
exports.clean_config_dir = clean_config_dir;

175 changes: 0 additions & 175 deletions src/test/unit_tests/jest_tests/test_list_object.test.js

This file was deleted.

Loading
Loading