Enhancements in transport-netty-http and properties related by jasonjoo2010 · Pull Request #161 · alibaba/Sentinel

jasonjoo2010 · 2018-09-28T11:08:44Z

Add:

VersionUtil to get version from jar
sentinel-transport-netty-http will also retry when port specified has been used

Fix:

LogBase repackage and support variable arguments
SentinelConfig remove duplicated JVM properties overriding

CLAassistant · 2018-09-28T11:08:50Z

All committers have signed the CLA.

codecov-io · 2018-09-28T11:16:25Z

Codecov Report

Merging #161 into master will decrease coverage by 0.65%.
The diff coverage is 44.11%.

@@             Coverage Diff              @@
##             master     #161      +/-   ##
============================================
- Coverage     51.15%   50.49%   -0.66%     
+ Complexity      822      818       -4     
============================================
  Files           139      140       +1     
  Lines          4737     4749      +12     
  Branches        672      674       +2     
============================================
- Hits           2423     2398      -25     
- Misses         2030     2066      +36     
- Partials        284      285       +1

Impacted Files	Coverage Δ	Complexity Δ
...com/alibaba/csp/sentinel/log/CommandCenterLog.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
.../main/java/com/alibaba/csp/sentinel/Constants.java	`83.33% <100%> (+3.33%)`	`1 <1> (ø)`	⬇️
...ava/com/alibaba/csp/sentinel/util/VersionUtil.java	`16.66% <16.66%> (ø)`	`1 <1> (?)`
...ain/java/com/alibaba/csp/sentinel/log/LogBase.java	`67.44% <50%> (-6.76%)`	`7 <2> (+2)`
...om/alibaba/csp/sentinel/config/SentinelConfig.java	`51.85% <66.66%> (+7.2%)`	`10 <0> (ø)`	⬇️
...n/java/com/alibaba/csp/sentinel/log/RecordLog.java	`76.92% <75%> (+0.45%)`	`4 <3> (ø)`	⬇️
...a/csp/sentinel/slots/statistic/base/Striped64.java	`26.04% <0%> (-23.96%)`	`5% <0%> (-2%)`
...a/csp/sentinel/slots/statistic/base/LongAdder.java	`17.02% <0%> (-14.9%)`	`4% <0%> (-4%)`
...a/csp/sentinel/slots/statistic/base/LeapArray.java	`68.96% <0%> (-5.18%)`	`17% <0%> (-1%)`

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 a25f25b...6ba04b6. Read the comment docs.

sczyh30 · 2018-09-28T11:38:23Z

...sentinel-transport-common/src/main/java/com/alibaba/csp/sentinel/transport/util/NetUtil.java

+        if (serverSocket != null) {
+            int finalPort = serverSocket.getLocalPort();
+            try {
+                serverSocket.close();


It's not a good idea to create server with available port, then close the socket and return the detected port. Once you closed the socket, the port may be released and other processes can take up this port before the port can be used by Sentinel transport later, which can cause error.

netty-http want to get a port number while simple-http want to get a ServerSocket.

So what about add a retry with new base port in netty-http? that's just like the old logic in simple-http.

That's okay to add retry logic in sentinel-transport-netty-http (though full of boilerplate), just ensure the server hold the connection where the port is available. You can put the NetUtil class to sentinel-transport-simple-http if it cannot be commonly reused :)

Or i have a solution and figuring it out if it's efficient. We can turn on REUSE_ADDR option and the port will remain TIME_WAIT for a while(even we hacked the kernel, in 1 second) and we can safely use it in a short time.

sczyh30 · 2018-09-28T11:42:06Z

sentinel-core/pom.xml

+            <plugin>
+                <groupId>org.apache.maven.plugins</groupId>
+                <artifactId>maven-jar-plugin</artifactId>
+                <version>3.0.2</version>


You can add the version to <properties>...</properties> like:

 <maven.jar.version>3.1.0</maven.jar.version>

yeah, I will add it as pluginManagement in parent pom

sczyh30 · 2018-09-28T11:43:16Z

sentinel-core/src/main/java/com/alibaba/csp/sentinel/log/LogBase.java

 import com.alibaba.csp.sentinel.util.PidUtil;

 /**
+ * Add support for placeholders in java.util.logging


Here is used to describe the whole class. It's better to remove the description and author information here.

sczyh30 · 2018-09-28T11:51:33Z

...e-http/src/main/java/com/alibaba/csp/sentinel/transport/command/SimpleHttpCommandCenter.java

 *
 * @author youji.zj
 */
+@SuppressWarnings("rawtypes")


Do not add this to class level. If really necessary, add to lower level (e.g. method level).

sentinel-core/src/main/java/com/alibaba/csp/sentinel/util/VersionUtil.java

sczyh30 · 2018-09-28T12:02:27Z

sentinel-core/src/main/java/com/alibaba/csp/sentinel/util/VersionUtil.java

+/**
+ * Get Sentinel version from MANIFEST.MF
+ * 
+ * @author jason<jason@dayima.com> @ Sep 28, 2018


You can remove the redundant author information (email, date) and only keep name here. Also, add a @since 0.2.1 here to indicate this is a new class since next version. Please also modify this in other new-created classes.

sczyh30 · 2018-10-09T11:35:43Z

Hi, can you resolve the conflicts?

jasonjoo2010 · 2018-10-09T12:05:03Z

Hi, can you resolve the conflicts?

It has been done

sczyh30

The CI indicates a build failure. Could you please resolve it?

sczyh30 · 2018-10-09T13:11:05Z

sentinel-core/src/main/java/com/alibaba/csp/sentinel/log/RecordLog.java

+    /**
+     * Change log dir, the dir will be created if not exits
+     */
+    public static void resetLogBaseDir(String baseDir) {


The reset function has been deprecated and you can remove it from the log classes.

* VersionUtil to get version from jar * sentinel-transport-netty-http will also retry when port specified has been used Fix: * LogBase repackage and support variable arguments * SentinelConfig remove duplicated JVM properties overriding

jasonjoo2010 · 2018-10-10T01:24:19Z

@sczyh30 all done last night

sczyh30

LGTM

sczyh30 · 2018-10-16T03:24:32Z

Thanks for your contribution!

Signed-off-by: Eric Zhao <sczyh16@gmail.com>

) * sentinel-transport-netty-http will also retry when port specified has been used * LogBase support variable arguments * Remove duplicated JVM properties overriding in SentinelConfig * Add VersionUtil to get version from jar

Signed-off-by: Eric Zhao <sczyh16@gmail.com>

sczyh30 added the to-review To review label Sep 28, 2018

jasonjoo2010 force-pushed the log_properties branch from cfff90c to 0cd7c34 Compare September 28, 2018 11:19

sczyh30 requested changes Sep 28, 2018

View reviewed changes

jasonjoo2010 force-pushed the log_properties branch 2 times, most recently from fc48c91 to 2ff69f4 Compare September 29, 2018 03:00

sczyh30 requested changes Oct 9, 2018

View reviewed changes

Add:

3df907e

* VersionUtil to get version from jar * sentinel-transport-netty-http will also retry when port specified has been used Fix: * LogBase repackage and support variable arguments * SentinelConfig remove duplicated JVM properties overriding

jasonjoo2010 force-pushed the log_properties branch 2 times, most recently from 3e8e44f to 6ba04b6 Compare October 9, 2018 18:24

merge from master

6ba04b6

sczyh30 approved these changes Oct 16, 2018

View reviewed changes

sczyh30 merged commit 0916a9b into alibaba:master Oct 16, 2018

sczyh30 removed the to-review To review label Oct 16, 2018

sczyh30 added a commit that referenced this pull request Oct 16, 2018

Code enhancement for #161

7626bf4

Signed-off-by: Eric Zhao <sczyh16@gmail.com>

Arlmls pushed a commit to Arlmls/Sentinel that referenced this pull request Jan 8, 2019

Code enhancement for alibaba#161

2ef4d98

Signed-off-by: Eric Zhao <sczyh16@gmail.com>

Conversation

jasonjoo2010 commented Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

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!

Uh oh!

sczyh30 Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sczyh30 commented Oct 9, 2018

Uh oh!

jasonjoo2010 commented Oct 9, 2018

Uh oh!

sczyh30 left a comment

Choose a reason for hiding this comment

Uh oh!

sczyh30 Oct 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonjoo2010 commented Oct 10, 2018

Uh oh!

sczyh30 left a comment

Choose a reason for hiding this comment

Uh oh!

sczyh30 commented Oct 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jasonjoo2010 commented Sep 28, 2018 •

edited

Loading

CLAassistant commented Sep 28, 2018 •

edited

Loading

codecov-io commented Sep 28, 2018 •

edited

Loading

sczyh30 Sep 28, 2018 •

edited

Loading

sczyh30 Oct 9, 2018 •

edited

Loading