optimize count items on get buckets query #369
Conversation
Hello sylvainsenechal,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
fc4e5e1 to
1c42fd5
Compare
CountItems/CountManager.js
Outdated
| method: 'CountManager::_setupQueue', | ||
| workInQueue: this.q.length(), | ||
| bucketInfo, | ||
| bucketName: bucketInfo._name, |
There was a problem hiding this comment.
first little fix : logging bucketInfo would try to log a massive object with quite a lot of useless stuff, and an error "2026-01-20T14:08:08.645643941Z stdout F "[unable to serialize, circular reference is too complex to analyze]"
I think it's reasonable to only log this
There was a problem hiding this comment.
we may want to log a few other fields to fully identify the bucket?
| bucketName: bucketInfo._name, | |
| bucketInfo: { name: bucketInfo._name, creationDate: bucketInfo._ creationDate, uid: bucketInfo._uid }, |
@benzekrimaha what do you think?
There was a problem hiding this comment.
In case we are not sure which information we want to log, use this method. It will surely provide more information than needed, but it avoids circular references.
There was a problem hiding this comment.
I added some more info, and used getters
| }); | ||
| const usersBucketCreationDatesArray = await cursorUsersBucketCreationDates.toArray(); | ||
| return usersBucketCreationDatesArray | ||
| .reduce((map, obj) => ({ ...map, [obj._id]: obj.value.creationDate }), {}); |
There was a problem hiding this comment.
The real problem, found by Maël : It's not even that we are querying all the buckets when we only need one (the perf in our test are still ok when doing this), but it's the reduce with spread operator : making a copy of the map on each iterations. Changing this from reduce to a simple loop reduce script run time from a few minutes to 2 seconds -_-
There was a problem hiding this comment.
btw, this function isn't used anymore, because as it turns out later, we don't need to query all buckets info, but only the info of a specific bucket and we have its name, so we can use another function for that.
I still kept this function, it's associated with tests, don't wanna touch too many things, anyways we are supposed to try to replace this script with SUR
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development/1.17 #369 +/- ##
====================================================
- Coverage 43.76% 43.72% -0.05%
====================================================
Files 84 84
Lines 5961 5953 -8
Branches 1256 1254 -2
====================================================
- Hits 2609 2603 -6
+ Misses 3306 3304 -2
Partials 46 46 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
utils/S3UtilsMongoClient.js
Outdated
| }, {}); | ||
| const inflightsMap = {}; | ||
| for (const inflight of inflights) { | ||
| if (!inflight?._id) { |
There was a problem hiding this comment.
is this a real case?
I don't think it can happen with mongodb, having an id is mandatory?
There was a problem hiding this comment.
right im too defensive here
utils/S3UtilsMongoClient.js
Outdated
| }); | ||
| }); | ||
| } catch (err) { | ||
| if (err && err.message === 'Bucket entry not found') { |
There was a problem hiding this comment.
getUsersBucketCreationDate is used in 2 places only : here and in collectucketMetricsAndUpdate, and it seems to me that in both case we ignore missing bucket info.
Also in that function, we explicitely detect that case and raise an exception.
It thus seems that it would greatly simplify code to directly return "undefined" creation date when the bucket is not found, instead of raising an error; and raise an exception only for an actual error.
This way we don't need extra try/catch and error filtering...
There was a problem hiding this comment.
Yes I changed it to return undefined when nothing is found
CountItems/CountManager.js
Outdated
| method: 'CountManager::_setupQueue', | ||
| workInQueue: this.q.length(), | ||
| bucketInfo, | ||
| bucketName: bucketInfo._name, |
There was a problem hiding this comment.
In case we are not sure which information we want to log, use this method. It will surely provide more information than needed, but it avoids circular references.
43e1398 to
9e6723d
Compare
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
| ownerId, | ||
| }); | ||
| return cb(new Error('Bucket entry not found')); | ||
| return cb(null, undefined); |
There was a problem hiding this comment.
why are we ignoring error now?
There was a problem hiding this comment.
maybe need a comment explaining the behavior of this function on "not found" (in function's doc header?)
There was a problem hiding this comment.
yeah I changed it following your recommandations, as it is easier to handle the "no data case" like this
9e6723d to
db0e5b3
Compare
|
/create_integration_branches |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option The following options are set: create_integration_branches |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: create_integration_branches |
|
/approve |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: approve, create_integration_branches |
|
/approve |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue S3UTILS-219. Goodbye sylvainsenechal. |
ISSUE: S3UTILS-219