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.

Sign up for GitHub

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

Jump to bottom

Delay flush if the watch queue has pending items #26085

Merged
merged 1 commit into from
May 28, 2016
Merged

Delay flush if the watch queue has pending items #26085

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 5 additions & 2 deletions pkg/apiserver/watch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,13 +168,14 @@ func (s *WatchServer) ServeHTTP(w http.ResponseWriter, req *http.Request) {
flusher.Flush()

buf := &bytes.Buffer{}
ch := s.watching.ResultChan()
for {
select {
case <-cn.CloseNotify():
return
case <-timeoutCh:
return
case event, ok := <-s.watching.ResultChan():
case event, ok := <-ch:
if !ok {
// End of results.
return
Expand All @@ -196,7 +197,9 @@ func (s *WatchServer) ServeHTTP(w http.ResponseWriter, req *http.Request) {
// client disconnect.
return
}
flusher.Flush()
if len(ch) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not convinced about this change. If the chrun in the cluster is big enough, ch can potentially never be empty.

I think we should flush no matter what at least 1 per X milliseconds.

Copy link
Contributor

Choose a reason for hiding this comment

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

see my suggestion. we can have a simple pending number.

Copy link
Contributor

Choose a reason for hiding this comment

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

also shouldnt http layer will do a auto flush if the http buffer is getting large?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Flush will happen under the covers for us - if we had enough items that the
cluster would never be full, we would never have to flush because Go would
be flushing for us.

On Mon, May 23, 2016 at 1:11 PM, Wojciech Tyczynski <
notifications@github.com> wrote:

In pkg/apiserver/watch.go
#26085 (comment)
:

@@ -196,7 +197,9 @@ func (s *WatchServer) ServeHTTP(w http.ResponseWriter, req *http.Request) {
// client disconnect.
return
}

  •       flusher.Flush()

  •       if len(ch) == 0 {

I'm not convinced about this change. If the chrun in the cluster is big
enough, ch can potentially never be empty.

I think we should flush no matter what at least 1 per X milliseconds.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/26085/files/c4bec1585fe9c31303c928c11ac45e380d2eaf77#r64252864

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To your last question, yes, we merely want to ensure we always flush if we
receive the "last" event. In all other cases Go will handle flushing for
us.

On Mon, May 23, 2016 at 1:14 PM, Xiang Li notifications@github.com wrote:

In pkg/apiserver/watch.go
#26085 (comment)
:

@@ -196,7 +197,9 @@ func (s *WatchServer) ServeHTTP(w http.ResponseWriter, req *http.Request) {
// client disconnect.
return
}

  •       flusher.Flush()

  •       if len(ch) == 0 {

also shouldnt http layer will do a auto flush if the http buffer is
getting large?

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/26085/files/c4bec1585fe9c31303c928c11ac45e380d2eaf77#r64253208

Copy link
Member

Choose a reason for hiding this comment

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

Do we have any guarantee on how frequently it will happen?
I know that this will happen, but if it waits for say tens of MB, that may not be good enough for us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wouldn't wait - once you reached the net/http buffer you'd get written.
I think the buffer is 32kb or 64kb by default.

On Mon, May 23, 2016 at 1:22 PM, Wojciech Tyczynski <
notifications@github.com> wrote:

In pkg/apiserver/watch.go
#26085 (comment)
:

@@ -196,7 +197,9 @@ func (s *WatchServer) ServeHTTP(w http.ResponseWriter, req *http.Request) {
// client disconnect.
return
}

  •       flusher.Flush()

  •       if len(ch) == 0 {

Do we have any guarantee on how frequently it will happen?
I know that this will happen, but if it waits for say tens of MB, that may
not be good enough for us.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/26085/files/c4bec1585fe9c31303c928c11ac45e380d2eaf77#r64254422

Copy link
Member

Choose a reason for hiding this comment

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

ok - SGTM;
thanks for clarification

flusher.Flush()
}

buf.Reset()
}
Expand Down