-
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
ZENKO-1124: mongo listing, avoid to loop #563
Conversation
Hello jeremyds,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:
Peer approvals must include at least 1 approval from the following list: |
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.
I have a hard time understanding the issue here.
My guess is that it's related to the changes in #559
That being said, with what I read on the other PR, I fear that we are created new corner cases where NextMarker isn't properly updated, and we end up with a skipping() == SKIP_NONE
; and I'm also unsure about the effect of that on this code.
Need an offline conversation to clarify this.
@DavidPineauScality This is related to https://scality.atlassian.net/browse/ZENKO-1124 , observed at a customer. |
I understand that, but my issue is that I don't understand how the new/alternative fix in #559 works, and I'm worried whether the fix here should also land in MetaData. |
Oh I was only commenting on what this issue is related to. As for the changes, I don't understand them either, it's better to schedule a call with @jeremyds. |
In theory @DavidPineauScality you indeed have to change Metadata code. We may factorize this function in Arsenal though since it is pure common code. |
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.
I will be discussing the solution offline with @jeremyds . I built a custom image and verified that the fix works.
Ok, thanks for the heads up. |
Sorry guys, I did not communicate well on the 2 PR. The other one, the fix in delimiterMaster, does not work. I tried to find a solution in the delimiterMaster code but each solution I found introduced (sometime subtle) regressions or behavior changes I was not confident with. Vianney closed the other PR, I should have done it yesterday. |
566e3fd
to
b306e20
Compare
758bca6
to
be6263d
Compare
lib/algos/list/skip.js
Outdated
class Skip { | ||
/** | ||
* @param {Object} params - skip parameters | ||
* @param {Object} extension - delimiter extension used (required) |
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.
Minor: This looks like additional arguments to the class. Can you update to make it apparent they are properties of params
?
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.
done
// eslint-disable-next-line no-param-reassign | ||
params.gt = undefined; | ||
// eslint-disable-next-line no-param-reassign | ||
params.gte = range; |
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.
Minor: Could be a bit cleaner to create a new object and reassign these properties rather than disabling the linter.
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.
Done. I saw it so much in the code than I thought it is a good practice :S
be6263d
to
5687a48
Compare
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.
I'm good with the logic, I have minor comments on the style though, feel free to ignore if time is short.
const stream = new MongoReadStream(c, params, | ||
params.mongifiedSearch); | ||
|
||
skip.setListingEndCb(() => { |
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.
minor: if those are mandatory callbacks, maybe they should be set directly in the constructor params instead of setters?
} | ||
|
||
setListingEndCb(cb) { | ||
this.listingEndCb = cb; |
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.
listingEndCb
and skipRangeCb
contracts should be documented explicitly, since they are essential to the behavior of the listing extension.
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 |
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue ZENKO-1124. Goodbye jeremyds. |
Note that I can still identify one corner-case (I don't see a solution ATM, tbh): |
No description provided.