Bugfix/s3 utils 239 fix count items bucket logging#394
Conversation
Hello benzekrimaha,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## development/1.16 #394 +/- ##
====================================================
+ Coverage 45.23% 45.29% +0.06%
====================================================
Files 85 85
Lines 6046 6051 +5
Branches 1282 1282
====================================================
+ Hits 2735 2741 +6
+ Misses 3265 3264 -1
Partials 46 46 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| const { deserializeBigInts, serializeBigInts } = require('./utils/utils'); | ||
|
|
||
| const PENSIEVE = 'PENSIEVE'; | ||
| const unsupportedSerializedBucketInfoFields = [ |
There was a problem hiding this comment.
could have kept the comment about why we do this 🤔
maeldonn
left a comment
There was a problem hiding this comment.
Once normalizeBucketInfoForCountItems updated, it will be good for me !
| const { deserializeBigInts, serializeBigInts } = require('./utils/utils'); | ||
|
|
||
| const PENSIEVE = 'PENSIEVE'; | ||
| const unsupportedSerializedBucketInfoFields = [ |
There was a problem hiding this comment.
nit:
| const unsupportedSerializedBucketInfoFields = [ | |
| const UNSUPPORTED_SERIALIZED_BUCKET_INFO_FIELDS = [ |
| '_websiteConfiguration', | ||
| ]; | ||
|
|
||
| function normalizeBucketInfoForCountItems(bucketInfoObj) { |
There was a problem hiding this comment.
Using object spread inside a reduce loop creates a brand new copy of the object on every iteration, leading to big time complexity and heavy memory churn. We can optimize this by cloning the object just once and using a for...of loop to safely update the properties.
| function normalizeBucketInfoForCountItems(bucketInfoObj) { | |
| function normalizeBucketInfoForCountItems(bucketInfoObj) { | |
| const normalized = { ...bucketInfoObj }; | |
| for (const field of unsupportedSerializedBucketInfoFields) { | |
| if (normalized[field]) { | |
| normalized[field] = null; | |
| } | |
| } | |
| return normalized; | |
| } |
In the PR we are nullifying _bucketLoggingStatus which is safe for count-items specifically: that this metadata is unrelated to object count / storage usage computation and only exists on the BucketInfo object because it is part of the full bucket metadata model.
The implementation we now have is cleaner and makes this explicit by naming the fields as unsupported serialized BucketInfo fields for count-items.
The rationale being that : class-backed fields that count-items does not consume should not be rehydrated in the worker path just to count object metadata.
Issue: S3UTILS-239