[TACHYON-62] Netty

[dcapwell]

Netty implementation of the data server.

This patch is not done yet. Its intent is to show the work I have right now for commenting.

This PR should not be merged until performance results come in.

[AmplabJenkins]

Can one of the admins verify this patch?

[dcapwell]

Having a hard time debugging why the String checkpointPath = TFS.getUfsPath(FILE.FID); in RemoteBlockInStream is "" when I run with netty, but has content with DataServer.

Anyone have an idea?

[dcapwell]

OK, found the issue. Decoding was ignoring the type so the block I'd was wrong. Consuming the type fixed the issue

[dcapwell]

Tests pass locally, will update when I get home

[mubarak]

I think that DataServerHandler event handler methods
in a different thread than an I/O thread so that the I/O thread is not blocked by
a time-consuming task.

 static final EventExecutorGroup group = new DefaultEventExecutorGroup(16); //number of Threads

 ChannelPipeline pipeline = ch.pipeline();

 pipeline.addLast("blockRequestDecoder", new BlockRequest.Decoder());
 pipeline.addLast("blockRequestEncoder", new BlockResponse.Encoder());
 pipeline.addLast(group, "dataServerHandler", new DataServerHandler(locker));

[rxin]

FYI I just submitted a similar pull request for Spark: apache/spark#1907

I think it does a lot of things you wanted to do. Would be great if you can review that one as well.

[dcapwell]

Thanks @rxin, I will take a look. I was looking at github.com/twitter/finagle and https://github.com/facebook/nifty as examples, but they are on 3.x, which is very different than 4.x

[hsaputra]

Add Javadoc to the class declaration

[hsaputra]

Missing Apache license header

[dcapwell]

Can we move to MIT? They don't care about this stuff!

=D

[dcapwell]

added

[dcapwell]

@mubarak, @hsaputra, @haoyuan, @rxin I finally did the testing to see how things perform, and have updated the code to reflect it. I think the patch is ready for a first real round of review.

I have made a few perf changes that I want to test out tomorrow, but that shouldn't effect the patch as a whole.

Few things I want to point out before you guys review:

1. I am keeping the old NIO implementation and have made it enableable view a config.
2. old NIO code was renamed and moved to the nio package.
3. only change to (NIO)DataServer is that the thread running it is now owned by it and not TachyonWorker. No other changes to these files

[haoyuan]

@yuzhu @RongGu

[rxin]

why would you need MAPPED ever?

[dcapwell]

Under some load I was seeing better results. Region is all around better
but thought it would be good to support both if needed.
On Aug 19, 2014 11:56 PM, "Reynold Xin" [email protected] wrote:

In core/src/main/java/tachyon/worker/netty/FileStreamType.java:

• * Unless required by applicable law or agreed to in writing, software
• * distributed under the License is distributed on an "AS IS" BASIS,
• * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
• * See the License for the specific language governing permissions and
• * limitations under the License.
• /
  +package tachyon.worker.netty;
  +
  +/*
• * When streaming files over the network, the implementation to use.
• */
  +public enum FileStreamType {
• /**
• * Create a memory-mapped buffer and send that over the network.
• */
• MAPPED,

why would you need MAPPED ever?

—
Reply to this email directly or view it on GitHub
https://github.com/amplab/tachyon/pull/333/files#r16461112.

[dcapwell]

@haoyuan, I believe I have addressed all your comments.

[hsaputra]

Sorry, but just noticed that if this is not set then the default sets null?
Do you know what Netty sets the socket send buffer if we don't set manually?

[dcapwell]

Netty docs redirect to http://docs.oracle.com/javase/7/docs/api/java/net/StandardSocketOptions.html?is-external=true#SO_SNDBUF, which says system dependent.

[hsaputra]

Cool, so I would suggest to just update our doc accordingly to make it clear.
Also not sure if you have seen this link http://fasterdata.es.net/host-tuning/

[dcapwell]

thanks for the link

[dcapwell]

updated config doc to be more descriptive.

[hsaputra]

Do final pass and add comment about socket buffer sizes.
After @haoyuan style recommendation, it is LGTM.
Thanks!

[haoyuan]

note. let them equal to null here.

[dcapwell]

how come? Java will give a warning that its not needed.

[haoyuan]

Thanks!

[dcapwell]

Thanks for adding it! Thanks everyone for the reviews!