Fix the queue when reprioritize #2339
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
Testing on Docs now. |
masaori335
reviewed
Aug 7, 2017
| { | ||
| node->parent->children.remove(node); | ||
| if (node->queued) { | ||
| node->parent->queue->erase(node->entry); |
Contributor
There was a problem hiding this comment.
If the queue of parent become empty by this operation, the queued flag of parent should also be false.
We should do same operations to parent of parent recursively.
So I think we should walk up the tree to correct queue and queued flag of parents here. WDYT?
masaori335
reviewed
Aug 7, 2017
| tree->reprioritize(1, 7, true); | ||
|
|
||
| box.check(node_a->queue->in(node_f->entry), "F should be in A's queue"); | ||
| // do we need to reset C's queued flag since C's queue is empty ?? |
Contributor
There was a problem hiding this comment.
I think so. In case for C is reprioritized. IMO, walking up the tree in _change_parent will fix this.
Member
Author
|
I'll updated after Docs Test. |
Contributor
|
Still tripping up on ASAN. :-/ |
Member
Author
|
Hi @masaori335 Can you please review again ! |
masaori335
previously approved these changes
Aug 8, 2017
Contributor
masaori335
left a comment
masaori335
left a comment
There was a problem hiding this comment.
Looks good to me 👍
But not sure this will fix the ASan issue:p
Contributor
|
If this still doesn't fix the ASAN issue, do we still want this for 7.1.1 ? |
Contributor
|
Fwiw, it still triggers on Docs with this :-). |
Member
Author
|
May be we need to block this pr until I address or reproduce this case. |
Contributor
|
I don't know how to reproduce it, but I was browsing docs.trafficserver from Safari and Chrome at the same time, and it crashed. Not sure if it was me, or someone else though :-). |
Contributor
|
I marked this as WIP for now, so we don't accidentally merge it. |
Member
Author
|
👍 |
Member
Author
|
Hi @zwoop @masaori335 Updated ! Can you please review again !! |
Member
Author
|
Only fixed the conflicts ! |
masaori335
approved these changes
Aug 13, 2017
Member
Author
|
@zwoop I think this pr is ready to launch ! Can you please backport to 7.1.x |
Contributor
|
Can we hold off on this one to 7.1.2 ? Or do we need it for 7.1.1 ? I'm just about ready to make the 7.1.1 RC, and this has not been tested particularly well. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pr tries to fix the #2323.