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 bufferchain for HTTP2 stream #23

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

peacocktrain
Copy link

Bufferchain is added for http2 stream and Iobuffer is implemented

@codecov-io
Copy link

codecov-io commented Nov 17, 2020

Codecov Report

Merging #23 (ea09cb0) into master (327992a) will increase coverage by 0.65%.
The diff coverage is 50.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #23      +/-   ##
==========================================
+ Coverage   63.00%   63.65%   +0.65%     
==========================================
  Files          17       18       +1     
  Lines         819      908      +89     
==========================================
+ Hits          516      578      +62     
- Misses        248      276      +28     
+ Partials       55       54       -1     
Impacted Files Coverage Δ
buffer/bufferchain.go 50.87% <50.87%> (ø)
buffer/iobuffer.go 76.15% <0.00%> (-0.32%) ⬇️
log/errorlog.go 55.00% <0.00%> (ø)
log/logger.go 51.81% <0.00%> (+1.33%) ⬆️
log/roller.go 78.49% <0.00%> (+8.75%) ⬆️
utils/goroutine.go 92.30% <0.00%> (+23.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 327992a...ea09cb0. Read the comment docs.

@taoyuanyuan taoyuanyuan requested a review from cch123 November 17, 2020 09:10
@cch123
Copy link
Contributor

cch123 commented Nov 17, 2020

please fix the golang-ci lint warnings~

这个 PR 的背景和上下文是啥?

@nejisama
Copy link
Collaborator

nejisama commented Dec 9, 2020

  1. 我看改动并没有更新依赖的地方,为什么更新了那么多依赖
  2. golint的报错都需要处理一下

@nejisama
Copy link
Collaborator

nejisama commented Dec 9, 2020

看了下是引入testify 导致的,pkg这个库 希望尽可能少的依赖,避免别的库引用的时候依赖过多,建议修改掉

@nejisama
Copy link
Collaborator

这个月不处理完,这个PR就关闭了哈

@peacocktrain
Copy link
Author

这个PR 是为 http2 流式请求建立一个 bufferchain,downstream的buffer在这个队列中直接送到upstream消费

@peacocktrain
Copy link
Author

这个pr能过,再更新修改mosnhttp2流式的哪个pr

@nejisama
Copy link
Collaborator

我看把vendor代码改了,但是go.sum没有改,应该不是正常的改动方式

@nejisama
Copy link
Collaborator

请按照标准流程来处理go.mod相关的文件

@peacocktrain
Copy link
Author

请按照标准流程来处理go.mod相关的文件

go.sum 已回退

}

// NewIoBufferChain returns *bufferChain.
func NewIoBufferChain(capacity int) *ioBufferchain {
Copy link
Contributor

Choose a reason for hiding this comment

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

return IoBuffer

// chain is full conn goutine wait 1s to consumer
ticker := time.NewTicker(1 * time.Second)
select {
case <-bc.errChan:
Copy link
Contributor

Choose a reason for hiding this comment

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

ticker.Stop()

return len(bytes), nil
default:
// chain is full conn goutine wait 1s to consumer
ticker := time.NewTicker(1 * time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

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

should be time.NewTimer() ?

case <-bc.errChan:
return
default:
close(bc.errChan)
Copy link
Contributor

Choose a reason for hiding this comment

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

we need pass error code, such as ErrWriteCovered

* ioBufferchain
* For HTTP2 stream, in order not to break the structure-adaptation interface.
*/
type ioBufferchain struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Disable PutIoBuffer()

if p, _ := buf.(*pipe); p != nil {

// ErrWriteCovered bufferchain queue is full.
var ErrWriteCovered = errors.New("chain write covered")

const defaultCapacity = 1 << 9
Copy link
Contributor

Choose a reason for hiding this comment

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

too big ...

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.

5 participants