Skip to content
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

Add separate send queue for kbfs file edit messages skipping the outbox #23530

Merged
merged 5 commits into from
Apr 9, 2020

Conversation

joshblum
Copy link
Member

@joshblum joshblum commented Apr 6, 2020

No description provided.

@joshblum joshblum requested a review from mmaxim April 6, 2020 23:37
@strib
Copy link
Contributor

strib commented Apr 7, 2020

Can you please wait for me to review this as well before you merge? I'll look tomorrow.

@strib strib self-requested a review April 7, 2020 02:26
@mmaxim
Copy link
Contributor

mmaxim commented Apr 7, 2020

Probably can't work this way, I'd like the calls from KBFS to still return to it immediately.

@joshblum joshblum force-pushed the joshblum/kbfs-queue-HOTPOT-2100 branch from e13b398 to 26600b5 Compare April 7, 2020 14:32
Copy link
Member Author

@joshblum joshblum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for the large diff here, thought deliverer was better suited in a separate file. marked the actual changes with comments


s.delivering = true
s.shutdownCh = make(chan struct{})
s.eg.Go(func() error { return s.deliverLoop(s.shutdownCh) })
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to support multiple loops

s.Debug(ctx, "stopping")
close(s.shutdownCh)
s.delivering = false
go func() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

block cb until loops finish

defer s.Trace(ctx, func() error { return err }, "Queue")()

// KBFSFILEEDIT msgs skip the traditional outbox
if msg.ClientHeader.Conv.TopicType == chat1.TopicType_KBFSFILEEDIT {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

skip outbox for kbfs files

return true
}

func (s *Deliverer) kbfsDeliverLoop(shutdownCh chan struct{}) error {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

read off of kbfs queue and do send

@joshblum joshblum changed the title Limit the number of concurrent KBFS chat posts, use post local instead of nonblock rpc Add separte send queue for kbfs file edit messages skipping the outbox Apr 7, 2020
@joshblum joshblum changed the title Add separte send queue for kbfs file edit messages skipping the outbox Add separate send queue for kbfs file edit messages skipping the outbox Apr 7, 2020
@strib
Copy link
Contributor

strib commented Apr 7, 2020

With no more KBFS changes, there's no need to wait for my review anymore. But I'm curious how this fixes the actual deadlock in the original report. is it just an orthogonal fix that will slow down the processing of kbfs edit requests? Or does it fix the actual, persistent deadlock observed in that issue?

@joshblum
Copy link
Member Author

joshblum commented Apr 7, 2020

@strib I'll look further into the deadlock, this PR is just to address the load on the chat system.

Copy link
Contributor

@mmaxim mmaxim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diff is hard to parse, but I agree Deliverer deserves its own file. Hope it works!

@joshblum joshblum force-pushed the joshblum/kbfs-queue-HOTPOT-2100 branch from b316d3d to de28afa Compare April 9, 2020 14:33
@joshblum joshblum merged commit dc9c321 into master Apr 9, 2020
@joshblum joshblum deleted the joshblum/kbfs-queue-HOTPOT-2100 branch April 9, 2020 14:33
mmou pushed a commit that referenced this pull request Apr 10, 2020
…ox (#23530)

* Limit the number of concurrent KBFS chat posts, use post local instead of nonblock rpc

* pseudo nonblock

* deliverer

* Fix kbfs loop

* merge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants