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

limiting max bytes in stream comsume queue with the same host socket #1958

Merged
merged 1 commit into from
Nov 10, 2022
Merged

limiting max bytes in stream comsume queue with the same host socket #1958

merged 1 commit into from
Nov 10, 2022

Conversation

chenbay
Copy link
Contributor

@chenbay chenbay commented Oct 19, 2022

  1. Add socket_max_streams_unconsumed_bytes to socket for limiting max bytes in stream comsume queue with the same host socket.
  2. Add min_buffer_size to stream options and remain_buffer_size to message Feedback for stream buffer control.

@chenbay
Copy link
Contributor Author

chenbay commented Oct 19, 2022

@wwbmmm reopen for #1928 .

@chenbay chenbay force-pushed the socket_max_reveive_streams_buffer branch from 6de002a to 2913a93 Compare October 19, 2022 09:07
wwbmmm
wwbmmm reviewed Oct 20, 2022
View reviewed changes
src/brpc/stream.cpp Outdated

if (FLAGS_socket_max_streams_unconsumed_bytes > 0) {
_host_socket->_total_streams_unconsumed_size -= new_remote_consumed - _remote_consumed;
if (_host_socket->_total_streams_unconsumed_size <= FLAGS_socket_max_streams_unconsumed_bytes) {
Copy link
Contributor

Choose a reason for hiding this comment

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

条件是不是写反了，这里应该是 > ？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

是的，= =

src/brpc/stream.cpp Outdated
@@ -72,6 +74,11 @@ int Stream::Create(const StreamOptions &options,
s->_connected = false;
s->_options = options;
s->_closed = false;
s->_cur_max_buf_size = options.max_buf_size;
if (FLAGS_socket_max_streams_unconsumed_bytes > 0 && options.min_buf_size > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

需要判断 min_buf_size < max_buf_size吧，否则是无效配置

Copy link
Contributor Author

Choose a reason for hiding this comment

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

嗯可以改下，这里本来是考虑max_buf_size=0要不限制。

Copy link
Contributor

Choose a reason for hiding this comment

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

此处应使用s->_options.min_buf_size

src/brpc/stream_impl.h Outdated
@@ -114,6 +114,7 @@ friend class MessageBatcher;
bthread_mutex_t _congestion_control_mutex;
size_t _produced;
size_t _remote_consumed;
size_t _cur_max_buf_size;
Copy link
Contributor

Choose a reason for hiding this comment

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

有了min_buf_size和max_buf_size，这个叫_cur_buf_size是不是更合理。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

咋说呢，其实也是max_buf_size，毕竟少用也是可以的，，大概是capacity的意思。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

_cur_buf_size

我先改成_cur_buf_size把

@chenbay chenbay force-pushed the socket_max_reveive_streams_buffer branch from 2913a93 to ae1d397 Compare October 20, 2022 09:37
@chenbay
Copy link
Contributor Author

chenbay commented Oct 20, 2022

@wwbmmm ok

@wwbmmm
Copy link
Contributor

wwbmmm commented Oct 21, 2022

编译失败且有warning，@chenbay

src/brpc/stream.cpp:81:30: error: cannot assign to variable 'options' with
      const-qualified type 'const brpc::StreamOptions &'
        options.min_buf_size = 0;
        ~~~~~~~~~~~~~~~~~~~~ ^
src/brpc/stream.cpp:68:41: note: variable 'options' declared const here
int Stream::Create(const StreamOptions &options, 
                   ~~~~~~~~~~~~~~~~~~~~~^~~~~~~
src/brpc/stream.cpp:326:117: warning: comparison of integers of different signs:
      'size_t' (aka 'unsigned long') and 'int' [-Wsign-compare]
  ...(_options.max_buf_size == 0 || _cur_buf_size < _options.max_buf_size)) {
                                    ~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~
src/brpc/stream.cpp:327:64: warning: comparison of integers of different signs:
      'unsigned long' and 'int' [-Wsign-compare]
  ...> 0 && _cur_buf_size * 2 > _options.max_buf_size) {
            ~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~

@chenbay chenbay force-pushed the socket_max_reveive_streams_buffer branch from ae1d397 to cf0d535 Compare October 21, 2022 06:43
@chenbay
Copy link
Contributor Author

chenbay commented Oct 24, 2022

@wwbmmm why is 1 workflow awaiting approval？

@wwbmmm
Copy link
Contributor

wwbmmm commented Oct 24, 2022

@wwbmmm why is 1 workflow awaiting approval？

Because you are first-time contributor.

wwbmmm
wwbmmm reviewed Oct 24, 2022
View reviewed changes
src/brpc/stream.cpp Outdated
@@ -72,6 +74,11 @@ int Stream::Create(const StreamOptions &options,
s->_connected = false;
s->_options = options;
s->_closed = false;
s->_cur_max_buf_size = options.max_buf_size;
if (FLAGS_socket_max_streams_unconsumed_bytes > 0 && options.min_buf_size > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

此处应使用s->_options.min_buf_size

src/brpc/stream.cpp Outdated Show resolved Hide resolved
@chenbay chenbay force-pushed the socket_max_reveive_streams_buffer branch from cf0d535 to 1874f5a Compare October 24, 2022 06:41
@chenbay
Copy link
Contributor Author

chenbay commented Oct 25, 2022

@wwbmmm ok

wwbmmm
wwbmmm reviewed Oct 25, 2022
View reviewed changes
src/brpc/stream.h Outdated
// The max size of unconsumed data allowed at remote side.
// If |max_buf_size| <= 0, there's no limit of buf size
// default: 2097152 (2M)
int max_buf_size;
size_t max_buf_size;
Copy link
Contributor

Choose a reason for hiding this comment

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

最好不要改已有的字段吧
而且注释里也写了 <= 0，说明可能是负数
我意思是用max_buf_size的地方判断一下是否>0再用

Copy link
Contributor Author

Choose a reason for hiding this comment

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

也行吧，按原来int的逻辑吧。

最好不要改已有的字段吧 而且注释里也写了 <= 0，说明可能是负数 我意思是用max_buf_size的地方判断一下是否>0再用

Copy link
Contributor Author

Choose a reason for hiding this comment

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

最好不要改已有的字段吧 而且注释里也写了 <= 0，说明可能是负数 我意思是用max_buf_size的地方判断一下是否>0再用

@wwbmmm 原来==判断改成<=了

@chenbay chenbay force-pushed the socket_max_reveive_streams_buffer branch from 1874f5a to 8d56034 Compare October 25, 2022 08:50
@wwbmmm
Copy link
Contributor

wwbmmm commented Oct 25, 2022

LGTM

wasphin
wasphin reviewed Oct 25, 2022
View reviewed changes
src/brpc/stream.cpp Outdated Show resolved Hide resolved
@chenbay chenbay force-pushed the socket_max_reveive_streams_buffer branch from 8d56034 to 66a3542 Compare October 25, 2022 13:06
@chenbay chenbay force-pushed the socket_max_reveive_streams_buffer branch from 66a3542 to efdfb63 Compare November 2, 2022 03:22
@wwbmmm wwbmmm merged commit f4cd44a into apache:master Nov 10, 2022
@wwbmmm wwbmmm mentioned this pull request Nov 23, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wwbmmm wwbmmm wwbmmm left review comments

@wasphin wasphin wasphin left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chenbay @wwbmmm @wasphin