-
Notifications
You must be signed in to change notification settings - Fork 19
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
ft: ZENKO-433 add item count support incremental update and refresh #499
ft: ZENKO-433 add item count support incremental update and refresh #499
Conversation
Hello alexanderchan-scality,My role is to assist you with the merge of this Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
dd6da47
to
13e4f2d
Compare
PR has been updated. Reviewers, please be cautious. |
@bert-e help |
Help pageThe following options and commands are available at this time. Options
Commands
|
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
13e4f2d
to
70c7349
Compare
Do it again human slave!:point_right: :runner: (Oh and the pull request has been updated, by the way.)
PR has been updated. Reviewers, please be cautious. |
70c7349
to
2ce9db4
Compare
PR has been updated. Reviewers, please be cautious. |
const PREV = 'prev'; | ||
|
||
function deepCopyObject(obj) { | ||
return JSON.parse(JSON.stringify(obj)); |
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.
that looks costly, is it the best practice when it comes to deep copy of objects attributes without all the methods?
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.
Suprising as it is, the native JSON parser is pretty efficient, more efficient than recursively iterating over object properties and array elements (which creates temp JS-side objects in the process).
this.objects = setVal.objects; | ||
this.versions = setVal.versions; | ||
this.buckets = setVal.buckets; | ||
this.bucketList = [...setVal.bucketList]; |
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.
what is the benefit of the spread syntax instead of a direct affectation of this.bucketList? deep copy?
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.
It's a shallow copy and is equivalent to loop-pushing into an empty new array. Since this is setting a new counter snapshot it allows being "consistent" and not have a dangling reference to an array that can still be pushed into by who knows what callback is in the queue.
const { objName, versionId } = params; | ||
if (isUserBucket(collectionName)) { | ||
if (isUpdate) { | ||
return c.find({ |
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.
What is the complexity of this find operation? it looks like it perform a database request.
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.
It helps maintain counters accurate (because the update can have a different size than the version it's overwriting) and does a get on a version id. It's either this with full-scan every hour or full-scan way more often. So PUT/DELETE operations on versioned buckets go from 1 read + 1 write to 2 read + 1 write.
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
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 |
I have successfully merged the changeset of this pull request
Please check the status of the associated issue ZENKO-433. Goodbye alexanderchan-scality. |
Old version: #497