[REEF-362] Implement a Wake Transport using HTTP

[nhne]

Additional implementation for Wake via HTTP

• ~new package org.apache.reef.wake.remote.transport.netty.http~
• no new package created.
• followed default implementation of netty
• Added TransportHttpTest
• currently tested only for HTTP

JIRA: REEF-362

Pull Request This closes #1341

[DifferentSC]

@nhne How about adding tests on RemoteManager using HttpMessagingTransportFactory?

[nhne]

Added one Test on RemoteManager with HttpMessagingTransportFactory.

[taegeonum]

@nhne I've skimmed through this PR, but it looks like it has a bunch of duplicated codes. Can't we minimize the duplicated codes? Maybe we need to refactor the original code to do this.

[nhne]

I implmeneted in this way to avoid modify the original code. I just worried I can insert err code into original code and compatibility issues. I think refactoring the original code can be eliminate many duplicated code since It does not have much difference. What Do you think would be better?

[nhne]

@DifferentSC Thank you for reviewing my work! I'll check the things you reviewed.

[nhne]

@motus I appreciate for your kind and precise review! I'll check out all about these. thanks a lot!

[DifferentSC]

Thanks a lot for your detailed comments, @motus! We have decided to split this issue into two, by making a new JIRA issue for refactoring and changing styles of Transport. @nhne will work on implementing HTTP transport after the refactoring is done. Thanks!

[nhne]

@taegeonum Thanks for sharing detailed review! I'll check all you mentioned above. Thank you.

[nhne]

@taegeonum I just pushed a commit reflexing your review. Thank you for your kind review!

[nhne]

@taegeonum Sorry for late response. I was busy for work about GSoC and reserve force traing(예비군 훈련). I'll check your review now. Thank you for review!

Thank you for good suggestion! We have discussed the using enum in parameter issue with @DifferentSC and @taegeonum. Unfortunately, current REEF Tang does not support enum correctly by now.

I tested the suggestion but it raises tang exception that 'could not resolve the enum value`.

So for now, I implemented in the way that interface uses enum but, uses string internally. I'll check about other reviews now. Thanks a lot @motus !

[nhne]

This is error when I tried to apply enum as Tang parameter.

Caused by: org.apache.reef.tang.exceptions.NameResolutionException: Could not resolve TCP.  Search ended at prefix TCP This can happen due to typos in class names that are passed as strings, or because Tang is configured to use a classloader other than the one that generated the class reference (check your classpath and the code that instantiated Tang)
at org.apache.reef.tang.implementation.java.ClassHierarchyImpl.getAlreadyBoundNode(ClassHierarchyImpl.java:274)
	at org.apache.reef.tang.implementation.java.ClassHierarchyImpl.getNode(ClassHierarchyImpl.java:261)
	at org.apache.reef.tang.implementation.java.ClassHierarchyImpl.parse(ClassHierarchyImpl.java:171)
	... 35 more

[motus]

Oh, that's very unfortunate. I was sure that Tang has support for enums by now! Luckily, this is easy to fix: here's my pull request #1370 - I hope it will help to make your code a few lines shorter 😄

[nhne]

Great work @motus ! I'll fix this after pulling the master. Thanks you very much!

[bgchun]

@nhne Did you test this code on a YARN cluster? If not, please do so.

[nhne]

I have ran tests on a YARN clutser with pulling most recent master branch. It passed 36 tests successfully. I will keep testing on Yarn by commits.

[bgchun]

I think it's better to refactor the tests in this PR.

[taegeonum]

@bgchun Either way is fine with me :) so @nhne, could you please refactor the tests?

[nhne]

I have changed implementation of TransportFactory to use Tang for choosing protocols.

And I also merged TransportHttpTest into TransportTest. It was hard to avoid using duplicated codes in separated test because JUnit didn't allowed multiple constructor for any test class and I thought that "changing tpFactory by a set method in instance of a test class" is not a good idea.

@taegeonum I would be grateful if you share your idea! Thank you.

[taegeonum]

@nhne I think you could do the same refactoring on RemoteManagerTest.java, as you did on TransportTest :)

[nhne]

I did merged Http tests into TransportTest and RemoteManagerTest repectly.

[nhne]

Thanks for your review! @taegeonum. I'll reconsider the structure of Http Channels considering your review.

[nhne]

@taegeonum Sorry for very late commit! I have addressed the review. Would you please have a look?

[nhne]

My former implementation need buf as global variable, but now we don't have to keep it global, so I converted it into local. Also I fixed misusing of codec in NettyChannelInitializer and sync in NettyHttpLink. @taegeonum Thank you for your review!

[nhne]

Sorry for late check. I have committed about your review. Thank you very much! @taegeonum

[nhne]

I thought about this somewhile... If any listener expect any response without any parameter, intetionally empty request can be sent. Then, this check would be erroneous. So I removed the if statements.

Thank you! @taegeonum

[nhne]

@taegeonum I have changed copiedBuffer into wrappedBuffer. Also I commented about sync upper. Thank you for your review!

[nhne]

@taegeonum Thank you for sharing your idea for sync(). I have removed it from NettyHttpLink. Thank you!

[nhne]

@taegeonum fixed the LOG check. Thank you!

[nhne]

@taegeonum I have fixed hostAddress into host. also I found URI.create can cause an IllegalArgumentException on invalid address, so I added RemoteRuntimeException about this invalid URI. Thank you for finding this error!

[taegeonum]

@nhne LGTM except for the last comment :)