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

Enhancements in transport-netty-http and properties related #161

Merged
merged 2 commits into from
Oct 16, 2018

Conversation

jasonjoo2010
Copy link
Collaborator

@jasonjoo2010 jasonjoo2010 commented Sep 28, 2018

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
Copy link

CLAassistant commented Sep 28, 2018

CLA assistant check
All committers have signed the CLA.

@sczyh30 sczyh30 added the to-review To review label Sep 28, 2018
@codecov-io
Copy link

codecov-io commented Sep 28, 2018

Codecov Report

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

Impacted file tree graph

@@             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.

if (serverSocket != null) {
int finalPort = serverSocket.getLocalPort();
try {
serverSocket.close();
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-jar-plugin</artifactId>
<version>3.0.2</version>
Copy link
Member

Choose a reason for hiding this comment

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

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

<!-- Use the latest version here -->
<maven.jar.version>3.1.0</maven.jar.version>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, I will add it as pluginManagement in parent pom

@@ -24,7 +24,10 @@
import com.alibaba.csp.sentinel.util.PidUtil;

/**
* Add support for placeholders in java.util.logging
Copy link
Member

Choose a reason for hiding this comment

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

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

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

/***
* The simple command center provides service to exchange information.
*
* @author youji.zj
*/
@SuppressWarnings("rawtypes")
Copy link
Member

Choose a reason for hiding this comment

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

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

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

@sczyh30 sczyh30 Sep 28, 2018

Choose a reason for hiding this comment

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

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.

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

sczyh30 commented Oct 9, 2018

Hi, can you resolve the conflicts?

@jasonjoo2010
Copy link
Collaborator Author

Hi, can you resolve the conflicts?

It has been done

Copy link
Member

@sczyh30 sczyh30 left a comment

Choose a reason for hiding this comment

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

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

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

@sczyh30 sczyh30 Oct 9, 2018

Choose a reason for hiding this comment

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

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 jasonjoo2010 force-pushed the log_properties branch 2 times, most recently from 3e8e44f to 6ba04b6 Compare October 9, 2018 18:24
@jasonjoo2010
Copy link
Collaborator Author

@sczyh30 all done last night

Copy link
Member

@sczyh30 sczyh30 left a comment

Choose a reason for hiding this comment

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

LGTM

@sczyh30 sczyh30 merged commit 0916a9b into alibaba:master Oct 16, 2018
@sczyh30
Copy link
Member

sczyh30 commented Oct 16, 2018

Thanks for your contribution!

@sczyh30 sczyh30 removed the to-review To review label Oct 16, 2018
sczyh30 added a commit that referenced this pull request Oct 16, 2018
Signed-off-by: Eric Zhao <sczyh16@gmail.com>
Arlmls pushed a commit to Arlmls/Sentinel that referenced this pull request Jan 8, 2019
)

* 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
Arlmls pushed a commit to Arlmls/Sentinel that referenced this pull request Jan 8, 2019
Signed-off-by: Eric Zhao <sczyh16@gmail.com>
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.

4 participants