Fix the issue netty#3806 in 4.1 by jiafu1115 · Pull Request #4858 · netty/netty

jiafu1115 · 2016-02-10T05:52:16Z

Motivation:

fix the issue #3806

Modifications:

deprecated ChannelOption.WRITE_BUFFER_HIGH_WATER_MARK and ChannelOption.WRITE_BUFFER_LOW_WATER_MARK.
add one new option called ChannelOption.WRITE_BUFFER_WATER_MARK.
add one new class type: WriteBufferWaterMark with constructor WriteBufferWaterMark(int, int)
It can support setting both low and high water mark at same time so that it can break values limited caused by default values.

so the followed usage is recomment after this fix:

serverBootstrap.childOption(ChannelOption.WRITE_BUFFER_WATER_MARK, new WriteBufferWaterMark(32768, 65536));

Result:
after the fix, the high/low water mark values limits caused by default values are removed.

@deprecated

fix sonar reported issue "Add the missing @deprecated annotation"

jiafu1115 · 2016-02-10T06:55:17Z

The issues reported by sonar are reported for old code. so if I fix these issues in this pull request, it will involved much more code changes and scopes. So can I omit them (only fix some issues involved small change scope such as deprecated) and only focus on the new code changes?

fix sonar issue

trustin · 2016-02-10T12:45:36Z

@jiafu1115 It's safe to ignore the info-level comments. They are just there to 'inform' you. I disabled the check about the deprecated members to reduce the amount of the noise.

SonarQube Github plugin seems add the comments even for the unchanged lines as long as they are shown in the 'Files changed' view tab. e.g. this cone It's perfectly fine to ignore such a comment, although you may want to address it as well if it's trivial to fix.

What I don't like about the plugin is that it adds a summary comment which isn't very useful, and there seem to be no way to disable it for now.

trustin · 2016-02-10T12:47:10Z

+    private final int writeBufferLowWaterMark;
+    private final int writeBufferHighWaterMark;
+
+    public WriteBufferWaterMark() {


Could we please hide this from users? e.g. make it package-private.

jiafu1115 · 2016-02-11T01:17:50Z

@trustin Thanks for your suggestions and I had updated the source code, can you help to check again?

normanmaurer · 2016-02-11T01:19:53Z

+     * Set the {@link WriteBufferWaterMark} which is used for setting the high and low
+     * water mark of the write buffer.
+     */
+    ChannelConfig setWriteBufferWaterMark(WriteBufferWaterMark writeBufferWaterMark);


You will need to also "override" this in SocketChannelConfig and others to return the correct type so method chaining works.

done except SocketChannelConfig and ServerSocketChannelConfig.

You should also do this for these both.

never mind we can not do this as it will break the API.

remove duplicated blank line

…o patch-11

jiafu1115 · 2016-02-11T02:29:29Z

@normanmaurer
I had override new method into all subclass if existed old methods such as setWriteBufferHighWaterMark(int) in them. Thanks for checking them.
What's more, I see the SocketChannelConfig is the interface extends ChannelConfig and hadn't override the old methods such as setWriteBufferHighWaterMark(int). maybe it is late discovery bug for forget to override setWriteBufferWaterMark in SocketChannelConfig to support chaining.
WDYT?

in short, no override new methods: ServerSocketChannelConfig and SocketChannelConfig
due to they hadn't never override setWriteBufferHighWaterMark(int) and setWriteBufferLowWaterMark(int). I think it is issue, can you make sure of it or is there any special reason for it?

remove WRITE_BUFFER_WATER_MARK from getOptions

jiafu1115 · 2016-02-12T00:16:44Z

    private volatile boolean autoClose = true;
    private volatile int writeBufferHighWaterMark = 64 * 1024;
    private volatile int writeBufferLowWaterMark = 32 * 1024;
+    private volatile WriteBufferWaterMark writeBufferWaterMark = new WriteBufferWaterMark(writeBufferLowWaterMark,writeBufferHighWaterMark);


@trustin seems duplicated?

but I can't remove member: writeBufferHighWaterMark and writeBufferLowWaterMark due to old methods such as setWriteBufferHighWaterMark(int) can't modify WriteBufferWaterMark except I add package visible setMethod into WriteBufferWaterMark.

And if I remove member: writeBufferWaterMark, I can't keep same style to support getOption() except I return new WriteBufferWaterMark (writeBufferLowWaterMark ,writeBufferHighWaterMark ) for every getOption() call;

so here confused me. I am not sure my current implement look good. so WDYT?

jiafu1115 · 2016-02-12T00:24:35Z

    private volatile boolean autoClose = true;
    private volatile int writeBufferHighWaterMark = 64 * 1024;
    private volatile int writeBufferLowWaterMark = 32 * 1024;
+    private volatile WriteBufferWaterMark writeBufferWaterMark =


@trustin seems duplicated?
but I can't remove members: writeBufferHighWaterMark and writeBufferLowWaterMark due to old methods such as setWriteBufferHighWaterMark(int) can't modify WriteBufferWaterMark except I add package visible setMethod into WriteBufferWaterMark.

And if I remove member: writeBufferWaterMark, I can't keep same style to support getOption() except I return new WriteBufferWaterMark (writeBufferLowWaterMark ,writeBufferHighWaterMark ) for every getOption() call;

but someday, we can remove members: writeBufferHighWaterMark and writeBufferLowWaterMark
after deprecated set methods such as setWriteBufferHighWaterMark be removed.
so here confused me. I am not sure if my current implement look good. so WDYT?

I think you should remove the local members and just have package private set methods.

normanmaurer · 2016-03-24T17:29:16Z

@jiafu1115 let me pick this up and finish as we are on the sprint for 4.1.0.Final :)

normanmaurer · 2016-03-24T20:06:57Z

@jiafu1115 once you addressed my last comment please also squash the commits. After that I will cherry-pick.

Sorry for the delay!

normanmaurer · 2016-03-29T15:11:56Z

@jiafu1115 also please squash into one commit once ready.

fix the issue netty#3806 Modifications: deprecated ChannelOption.WRITE_BUFFER_HIGH_WATER_MARK and ChannelOption.WRITE_BUFFER_LOW_WATER_MARK. add one new option called ChannelOption.WRITE_BUFFER_WATER_MARK. add one new class type: WriteBufferWaterMark with constructor WriteBufferWaterMark(int, int) It can support setting both low and high water mark at same time so that it can break values limited caused by default values. so the followed usage is recommended after this fix: serverBootstrap.childOption(ChannelOption.WRITE_BUFFER_WATER_MARK, new WriteBufferWaterMark(32768, 65536)); Result: after the fix, the high/low water mark values limits caused by default values are removed.

…o patch-11

Motivation: fix the issue netty#3806 Modifications: deprecated ChannelOption.WRITE_BUFFER_HIGH_WATER_MARK and ChannelOption.WRITE_BUFFER_LOW_WATER_MARK. add one new option called ChannelOption.WRITE_BUFFER_WATER_MARK. add one new class type: WriteBufferWaterMark with constructor WriteBufferWaterMark(int, int) It can support setting both low and high water mark at same time so that it can break values limited caused by default values. so the followed usage is recommended after this fix: serverBootstrap.childOption(ChannelOption.WRITE_BUFFER_WATER_MARK, new WriteBufferWaterMark(32768, 65536)); Result: after the fix, the high/low water mark values limits caused by default values are removed.

…o patch-11

Motivation: fix the issue netty#3806 Modifications: deprecated ChannelOption.WRITE_BUFFER_HIGH_WATER_MARK and ChannelOption.WRITE_BUFFER_LOW_WATER_MARK. add one new option called ChannelOption.WRITE_BUFFER_WATER_MARK. add one new class type: WriteBufferWaterMark with constructor WriteBufferWaterMark(int, int) It can support setting both low and high water mark at same time so that it can break values limited caused by default values. so the followed usage is recommended after this fix: serverBootstrap.childOption(ChannelOption.WRITE_BUFFER_WATER_MARK, new WriteBufferWaterMark(32768, 65536)); Result: after the fix, the high/low water mark values limits caused by default values are removed.

…o patch-11

Motivation: Fix the issue netty#3806 Modifications: deprecated ChannelOption.WRITE_BUFFER_HIGH_WATER_MARK and ChannelOption.WRITE_BUFFER_LOW_WATER_MARK. add one new option called ChannelOption.WRITE_BUFFER_WATER_MARK. add one new class type: WriteBufferWaterMark with constructor WriteBufferWaterMark(int, int) It can support setting both low and high water mark at same time so that it can break values limited caused by default values. so the followed usage is recommended after this fix: serverBootstrap.childOption(ChannelOption.WRITE_BUFFER_WATER_MARK, new WriteBufferWaterMark(32768, 65536)); Result: after the fix, the high/low water mark values limits caused by default values are removed.

…o patch-11

jiafu1115 · 2016-03-30T05:08:58Z

@normanmaurer I had completed all the code change. It is first time for me to use squash method and I an't sure whether my operation is right. The squash commit is 551ad90
can you help to review it?
3ks.

normanmaurer · 2016-03-30T11:56:37Z

@jiafu1115 I did take care of squashing for you and did some more small-adjustments for proper CAS semantics and to ensure we only need to read one volatile field.

Here is the new PR which still lists you as author:
#5060

jiafu1115 · 2016-03-30T12:57:45Z

@normanmaurer Thank you very much!

jiafu1115 added 2 commits February 10, 2016 13:41

fix sonar reported issue

2244600

fix sonar reported issue "Add the missing @deprecated annotation"

fix the issue netty#3806 in 4.1

0a5e33b

jiafu1115 mentioned this pull request Feb 10, 2016

fix the issue netty#3806 in 4.1 #4822

Closed

fix sonar issue

c3ede0a

fix sonar issue

trustin reviewed Feb 10, 2016
View reviewed changes

improve according to lee's suggestions

f032f2b

normanmaurer reviewed Feb 11, 2016
View reviewed changes

jiafu1115 and others added 3 commits February 11, 2016 09:20

remove duplicated blank line

37c8451

remove duplicated blank line

override new add method add improve javadocs

419ee6f

Merge branch 'patch-11' of https://github.com/jiafu1115/netty.git int…

320cdef

…o patch-11

jiafu1115 added 4 commits February 11, 2016 10:30

fix check style

d102c68

override to support chain method

e2b39ea

remove WRITE_BUFFER_WATER_MARK from getOptions

d81878c

remove WRITE_BUFFER_WATER_MARK from getOptions

add getOption support.

be3ad0e

jiafu1115 reviewed Feb 12, 2016
View reviewed changes

fix check style

e44f122

jiafu1115 reviewed Feb 12, 2016
View reviewed changes

normanmaurer self-assigned this Mar 24, 2016

normanmaurer added this to the 4.1.0.Final milestone Mar 24, 2016

jiafu1115 added 6 commits March 29, 2016 17:04

add @deprecated

c4183db

add @deprecated

b4ea9d1

add @deprecated

88414c9

add @deprecated

80b0687

add @deprecated

dcaf759

add volatile to protected concurrent set.

5c5c365

jiafu1115 added 17 commits March 30, 2016 08:49

add into socket channel

8716f02

support method chain

725977d

support method chain

b3297d1

Update SocketChannelConfig.java

cac80a8

Update ServerSocketChannelConfig.java

92ce68a

Merge branch 'patch-11' of https://github.com/jiafu1115/netty.git int…

905d0e4

…o patch-11

Merge branch 'patch-11' of https://github.com/jiafu1115/netty.git int…

a01a018

…o patch-11

fix deprecated

e455221

fix deprecated

1601070

fix deprecated

c81e62f

Merge branch 'patch-11' of https://github.com/jiafu1115/netty.git int…

e8f5ec0

…o patch-11

fix sonar issue

d4188a9

Merge branch 'patch-11' of https://github.com/jiafu1115/netty.git int…

95c4bcb

…o patch-11

normanmaurer closed this Mar 30, 2016

trustin modified the milestones: 4.1.0.CR6, 4.1.0.Final Apr 2, 2016

Uh oh!

Conversation

jiafu1115 commented Feb 10, 2016

Uh oh!

jiafu1115 commented Feb 10, 2016

Uh oh!

trustin commented Feb 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiafu1115 commented Feb 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiafu1115 commented Feb 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

normanmaurer commented Mar 24, 2016

Uh oh!

normanmaurer commented Mar 24, 2016

Uh oh!

normanmaurer commented Mar 29, 2016

Uh oh!

jiafu1115 commented Mar 30, 2016

Uh oh!

normanmaurer commented Mar 30, 2016

Uh oh!

jiafu1115 commented Mar 30, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants