Github user revans2 commented on the pull request:
https://github.com/apache/incubator-storm/pull/250#issuecomment-55190671
For the most part things look really good.
There are a number of whitespace issues all throughout. Mostly looks like
tabs are in there when there should be spaces. This if fairly minor, but I
would like to fix them.
I would also like to see some basic sanity unit tests added as well. I
wrote a couple that all passed inside
storm-core/test/clj/backtype/storm/messaging/netty_unit_test.clj they are all
more or less like test-basic but with auth, auth-conf, and auth-int turned on.
Something like the following.
```
(deftest test-basic-auth
(let [req_msg (String. "0123456789abcdefghijklmnopqrstuvwxyz")
storm-conf {STORM-MESSAGING-TRANSPORT
"backtype.storm.messaging.netty.Context"
STORM-MESSAGING-NETTY-AUTHENTICATION true
STORM-MESSAGING-NETTY-BUFFER-SIZE 1024
STORM-MESSAGING-NETTY-MAX-RETRIES 10
STORM-MESSAGING-NETTY-MIN-SLEEP-MS 1000
STORM-MESSAGING-NETTY-MAX-SLEEP-MS 5000
STORM-MESSAGING-NETTY-SERVER-WORKER-THREADS 1
STORM-MESSAGING-NETTY-CLIENT-WORKER-THREADS 1
STORM-MESSAGING-NETTY-PROTECTION "auth"
TOPOLOGY-NAME "testing"
STORM-ZOOKEEPER-TOPOLOGY-AUTH-PAYLOAD "shhhh:don'ttell"
}
context (TransportFactory/makeContext storm-conf)
server (.bind context nil port)
client (.connect context nil "localhost" port)
_ (.send client task (.getBytes req_msg))
iter (.recv server 0 0)
resp (.next iter)]
(is (= task (.task resp)))
(is (= req_msg (String. (.message resp))))
(.close client)
(.close server)
(.term context)))
```
Things are also starting to get kind of complex with the different classes,
especially the Encoders/Decoders. It looks like there is some copy and paste
code shared between them. Would it be possible to try and combine some of the
code together again. Especially the part in MessageDecoder that appreas to be
the same in ClientUnwrapMessageDecoder and ServerUnwrapMessageDecoder?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
