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

Use multiple separate counters for the single service with multiple protocols. #222

Merged
merged 7 commits into from
Jul 13, 2018
Merged

Use multiple separate counters for the single service with multiple protocols. #222

merged 7 commits into from
Jul 13, 2018

Conversation

leizhiyuan
Copy link
Contributor

Motivation:

Fix sofastack/sofa-rpc-boot-projects#67

Modification:

Describe the idea and modifications you've done.

Result:

Fixes sofastack/sofa-rpc-boot-projects#67

@leizhiyuan leizhiyuan requested a review from ujjboy July 6, 2018 08:11
@leizhiyuan leizhiyuan changed the title fix multi protol publish fix multi protocol publish Jul 6, 2018
@codecov-io
Copy link

codecov-io commented Jul 9, 2018

Codecov Report

Merging #222 into 5.4 will increase coverage by 0.35%.
The diff coverage is 83.72%.

Impacted file tree graph

@@             Coverage Diff              @@
##                5.4     #222      +/-   ##
============================================
+ Coverage     71.13%   71.49%   +0.35%     
  Complexity      961      961              
============================================
  Files           341      341              
  Lines         13654    14238     +584     
  Branches       2208     2290      +82     
============================================
+ Hits           9713    10179     +466     
- Misses         2763     2865     +102     
- Partials       1178     1194      +16
Impacted Files Coverage Δ Complexity Δ
...y/sofa/rpc/bootstrap/DefaultProviderBootstrap.java 66.82% <83.72%> (+1.03%) 0 <0> (ø) ⬇️
...java/com/alipay/sofa/rpc/context/AsyncRuntime.java 53.84% <0%> (-9.8%) 0% <0%> (ø)
...alipay/sofa/rpc/api/future/SofaResponseFuture.java 55.17% <0%> (-6.37%) 5% <0%> (ø)
...pay/sofa/rpc/transport/http/SslContextBuilder.java 18.75% <0%> (-6.25%) 2% <0%> (ø)
...fa/rpc/bootstrap/dubbo/DubboProviderBootstrap.java 60.44% <0%> (-6.22%) 0% <0%> (ø)
...ay/sofa/rpc/protocol/telnet/HelpTelnetHandler.java 82.35% <0%> (-5.15%) 0% <0%> (ø)
...va/com/alipay/sofa/rpc/client/FailoverCluster.java 74.28% <0%> (-4.29%) 0% <0%> (ø)
...va/com/alipay/sofa/rpc/client/FailFastCluster.java 75% <0%> (-2.78%) 0% <0%> (ø)
...n/java/com/alipay/sofa/rpc/ext/ExtensionClass.java 77.77% <0%> (-2.23%) 0% <0%> (ø)
.../sofa/rpc/client/aft/impl/TimeWindowRegulator.java 81.91% <0%> (-2.18%) 7% <0%> (ø)
... and 122 more

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 6f3dec6...78d59f9. Read the comment docs.

String protocol = serverConfig.getProtocol();
String key = providerConfig.buildKey() + ":" + protocol;
AtomicInteger cnt = EXPORTED_KEYS.get(key); // 计数器
if (cnt != null && cnt.get() > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

这里应该记录下已经增加的计数器,而不是全部计数器。 例如发布A、B、C协议,发到 B 的时候失败,应该只需要减去 A 的计数器即可,B 和 C 的计数器都不需要减。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个抛出异常的时候,比较难记录.. register();因为这行代码可能跑错.

Copy link
Member

Choose a reason for hiding this comment

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

如果 register() 抛错, 那说明 A B C 都发布完了。 我举的例子应该是还在发 B 协议的时候。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

改掉了.

@@ -164,6 +176,7 @@ private void doExport() {
if (serverConfig.isAutoStart()) {
server.start();
}
hasExportedInCurrent.put(serverConfig.getProtocol(), true);
Copy link
Member

Choose a reason for hiding this comment

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

我看计数器加1的地方是在 131 行的地方加的,在这个位置记录加1的情况应该会有不对应的问题。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个地方是为了记录本次 export 成功了哪个协议.是一个局部的操作.


Assert.fail();
} catch (Throwable e) {
Assert.assertTrue(true);
Copy link
Member

Choose a reason for hiding this comment

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

这行没用

@ujjboy ujjboy changed the title fix multi protocol publish Use multiple separate counters for the single service with multiple protocols. Jul 13, 2018
@ujjboy ujjboy added the optimization Code optimization label Jul 13, 2018
@ujjboy ujjboy added this to the 5.4.3 milestone Jul 13, 2018
@ujjboy ujjboy merged commit b0b4176 into sofastack:5.4 Jul 13, 2018
@leizhiyuan leizhiyuan deleted the fix_multi_protocol branch November 9, 2018 03:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
optimization Code optimization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants