Skip to content

STORM-1038: Upgrade netty to 4.x in 1.x-branch - #1591

Closed
ooasis wants to merge 1 commit into
apache:1.x-branchfrom
ooasis:1.x-branch-netty4
Closed

STORM-1038: Upgrade netty to 4.x in 1.x-branch#1591
ooasis wants to merge 1 commit into
apache:1.x-branchfrom
ooasis:1.x-branch-netty4

Conversation

@ooasis

Copy link
Copy Markdown

This is to add the feature to 1.x-branch. The original PR for master branch is #728.

@ooasis

Copy link
Copy Markdown
Author

Performance test results are attached here.
perf_compare_netty_3vs4_1.x-branch.zip

Baseline: current head of 1.x-branch (e55684b)
Netty4: 57fbccc

In addition to default configuration,
io.netty.noPreferDirect=false
io.netty.allocator.type=pooled

following configurations are also tested for netty 4.x

-unpool
io.netty.allocator.type=unpooled. Ask Netty to not use pooled allocation for buffer

-nodirect
io.netty.noPreferDirect=true. Ask Netty to not use off-heap buffer allocation

-unpool-nodirect
io.netty.noPreferDirect=true
io.netty.allocator.type=unpooled

Due to the capacity of my laptop, I tested following loads: 2k, 5k, 10k. At 10k, the CPU usage reaches above 85% and failures started to appear at startup.

The performance data seems to suggest

  • no big difference is observed comparing before/after netty upgrades
    I suspect only a small percentage of traffic will cross worker instances (jvm) so Netty does not play a big role in performance measured
  • netty 4.x consumes a little more CPU and memory
    The small memory increment may be simply b/c netty 4.x pulled in more classes.
  • netty 4.x has lower CPU consumption on GC
    This is especially visible with the default configuration where off-heap buffer allocation is used.

@ooasis

Copy link
Copy Markdown
Author

Seems the Travis CI build will fail at random places which are not related to this PR.

For example the last build failed due to

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-remote-resources-plugin:1.2.1:process (default) on project flux-examples: Error resolving project artifact: Could not transfer artifact io.confluent:kafka-schema-registry-client:pom:1.0 from/to sonatype-apache (https://repository.apache.org/releases/): Connect to repository.apache.org:443 [repository.apache.org/207.244.88.143] failed: Connection timed out for project io.confluent:kafka-schema-registry-client:jar:1.0 -> [Help 1]

Any way to manually trigger a new CI build w/o code change?

-thanks

@HeartSaVioR

Copy link
Copy Markdown
Contributor

You can close and reopen this PR to retrigger. Btw, apache repository seems to be unstable recent days. I saw other build failures due to repository connectivity issue.

byte[] initialChallenge = saslNettyClient.saslResponse(new SaslMessageToken(new byte[0]));
LOG.debug("Sending initial challenge: {}", initialChallenge);
channel.write(new SaslMessageToken(initialChallenge));
channel.writeAndFlush(new SaslMessageToken(initialChallenge));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is it changed to use writeAndFlush?

@satishd

Copy link
Copy Markdown
Member

@hsun-cnnxty Channel#write is modified to Channel#writeAndFlush at multiple places. Does not that flush to the underlying stream without buffering and flush it when buffers are full?

@ooasis

Copy link
Copy Markdown
Author

@satishd nett 4.x has removed auto-flush in write() and it requires user to explicitly call flush() or writeAndFlush(), so we need to flush for the last message in the logic flow. Fortunately, the storm netty client does batching of messages itself and it does not flush for every message.

@ooasisooasis closed this Aug 12, 2016
@ooasisooasis reopened this Aug 12, 2016
@ooasisooasis closed this Aug 18, 2016
@ooasisooasis reopened this Aug 18, 2016
@isPositiveNumber
public static final String STORM_MESSAGING_NETTY_BUFFER_SIZE = "storm.messaging.netty.buffer_size";

/**

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These config descriptions are not descriptive. Without reading the code, I have no idea what this does.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Will take a look at how to improve. Any suggestions?

-thanks

@HeartSaVioR

Copy link
Copy Markdown
Contributor

Since Netty made 3.x version line EOL, I'd like to bump Netty to 4.0.x or even 4.1.x if there's no performance / resource usage issue.

@hsun-cnnxty
Please upmerge this. If you really don't mind, could you do performance tests again on two latest versions: 4.0.41.Final and 4.1.5.Final, too?

@ooasis

Copy link
Copy Markdown
Author

I will find some time this weekend to work on it.

-thanks

@ooasisooasis closed this Sep 24, 2016
@ooasisooasis reopened this Sep 24, 2016
@ooasisooasis closed this Sep 30, 2016
@ooasisooasis reopened this Sep 30, 2016
@HeartSaVioR

Copy link
Copy Markdown
Contributor

Please rebase and force push accordingly. Your PR now has all of the commits from other side.

You can give it an another try: 1) create new branch based on recent master or 1.x 2) cherry-pick your commits 3) checkout to your PR branch 4) reset your PR branch to the branch which you just worked 4) force push.

@ooasis

Copy link
Copy Markdown
Author

@HeartSaVioR
I rebased to 1.x-branch and also squashed the commits. Will attached the performance test results soon.

@ooasis

Copy link
Copy Markdown
Author

Tests are done to compare following two code bases:

  • baseline: current head of 1.x-branch (62476f5)
  • netty 4.1.x: 1.x-branch-netty4 (dd96d9f)

In summary:

  • netty 4.1.x is able to sustain higher throughput (20k/sec) than baseline (between 15k -20k/sec)
  • netty 4.1.x has lower GC activity which could be the reason that it has higher throughput**

Also tested different netty configurations regarding buffer allocation

  • use direct buffer or heap
  • use pooled or unpooled

Result shows the default setting works best.

Performance tests are done on my laptop with following config:

Model Name: MacBook Pro
Processor Name: Intel Core i7
Processor Speed: 2.3 GHz
Number of Processors: 1
Total Number of Cores: 4
L2 Cache (per Core): 256 KB
L3 Cache: 6 MB
Memory: 16 GB
System Version: OS X 10.10.5 (14F27)
Kernel Version: Darwin 14.5.0

The data collected is attached.
performance_compare_netty_1.x-branch.zip

@HeartSaVioR

Copy link
Copy Markdown
Contributor

@hsun-cnnxty
Sorry I lost tracking this.
From the last time I tested on my local machine (via ThroughputVsLatency), before patching this works better than after patching this. My test environment seems not stable.
Someone might need to test on this, and if he/she can run benchmark with multi machines that should be great.

d2r pushed a commit to d2r/storm that referenced this pull request Oct 16, 2018
We are closing stale Pull Requests to make the list more manageable.
Please re-open any Pull Request that has been closed in error.
Closesapache#608Closesapache#639Closesapache#640Closesapache#648Closesapache#662Closesapache#668Closesapache#692Closesapache#705Closesapache#724Closesapache#728Closesapache#730Closesapache#753Closesapache#803Closesapache#854Closesapache#922Closesapache#986Closesapache#992Closesapache#1019Closesapache#1040Closesapache#1041Closesapache#1043Closesapache#1046Closesapache#1051Closesapache#1078Closesapache#1146Closesapache#1164Closesapache#1165Closesapache#1178Closesapache#1213Closesapache#1225Closesapache#1258Closesapache#1259Closesapache#1268Closesapache#1272Closesapache#1277Closesapache#1278Closesapache#1288Closesapache#1296Closesapache#1328Closesapache#1342Closesapache#1353Closesapache#1370Closesapache#1376Closesapache#1391Closesapache#1395Closesapache#1399Closesapache#1406Closesapache#1410Closesapache#1422Closesapache#1427Closesapache#1443Closesapache#1462Closesapache#1468Closesapache#1483Closesapache#1506Closesapache#1509Closesapache#1515Closesapache#1520Closesapache#1521Closesapache#1525Closesapache#1527Closesapache#1544Closesapache#1550Closesapache#1566Closesapache#1569Closesapache#1570Closesapache#1575Closesapache#1580Closesapache#1584Closesapache#1591Closesapache#1600Closesapache#1611Closesapache#1613Closesapache#1639Closesapache#1703Closesapache#1711Closesapache#1719Closesapache#1737Closesapache#1760Closesapache#1767Closesapache#1768Closesapache#1785Closesapache#1799Closesapache#1822Closesapache#1824Closesapache#1844Closesapache#1874Closesapache#1918Closesapache#1928Closesapache#1937Closesapache#1942Closesapache#1951Closesapache#1957Closesapache#1963Closesapache#1964Closesapache#1965Closesapache#1967Closesapache#1968Closesapache#1971Closesapache#1985Closesapache#1986Closesapache#1998Closesapache#2031Closesapache#2032Closesapache#2071Closesapache#2076Closesapache#2108Closesapache#2119Closesapache#2128Closesapache#2142Closesapache#2174Closesapache#2206Closesapache#2297Closesapache#2322Closesapache#2332Closesapache#2341Closesapache#2377Closesapache#2414Closesapache#2469
d2r pushed a commit to d2r/storm that referenced this pull request Oct 16, 2018
We are closing stale Pull Requests to make the list more manageable.
Please re-open any Pull Request that has been closed in error.
Closesapache#608Closesapache#639Closesapache#640Closesapache#648Closesapache#662Closesapache#668Closesapache#692Closesapache#705Closesapache#724Closesapache#728Closesapache#730Closesapache#753Closesapache#803Closesapache#854Closesapache#922Closesapache#986Closesapache#992Closesapache#1019Closesapache#1040Closesapache#1041Closesapache#1043Closesapache#1046Closesapache#1051Closesapache#1078Closesapache#1146Closesapache#1164Closesapache#1165Closesapache#1178Closesapache#1213Closesapache#1225Closesapache#1258Closesapache#1259Closesapache#1268Closesapache#1272Closesapache#1277Closesapache#1278Closesapache#1288Closesapache#1296Closesapache#1328Closesapache#1342Closesapache#1353Closesapache#1370Closesapache#1376Closesapache#1391Closesapache#1395Closesapache#1399Closesapache#1406Closesapache#1410Closesapache#1422Closesapache#1427Closesapache#1443Closesapache#1462Closesapache#1468Closesapache#1483Closesapache#1506Closesapache#1509Closesapache#1515Closesapache#1520Closesapache#1521Closesapache#1525Closesapache#1527Closesapache#1544Closesapache#1550Closesapache#1566Closesapache#1569Closesapache#1570Closesapache#1575Closesapache#1580Closesapache#1584Closesapache#1591Closesapache#1600Closesapache#1611Closesapache#1613Closesapache#1639Closesapache#1703Closesapache#1711Closesapache#1719Closesapache#1737Closesapache#1760Closesapache#1767Closesapache#1768Closesapache#1785Closesapache#1799Closesapache#1822Closesapache#1824Closesapache#1844Closesapache#1874Closesapache#1918Closesapache#1928Closesapache#1937Closesapache#1942Closesapache#1951Closesapache#1957Closesapache#1963Closesapache#1964Closesapache#1965Closesapache#1967Closesapache#1968Closesapache#1971Closesapache#1985Closesapache#1986Closesapache#1998Closesapache#2031Closesapache#2032Closesapache#2071Closesapache#2076Closesapache#2108Closesapache#2119Closesapache#2128Closesapache#2142Closesapache#2174Closesapache#2206Closesapache#2297Closesapache#2322Closesapache#2332Closesapache#2341Closesapache#2377Closesapache#2414Closesapache#2469
Sign up for freeto 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

@ooasis@HeartSaVioR@satishd@knusbaum