-
Notifications
You must be signed in to change notification settings - Fork 19
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
Refactor Kinetic lib #24
Conversation
Old tests were buggy (HMAC problems) and we don't have guarantees on the messages contents validity. Now that we have trustable tests with a third-party simulator, we don't need them anymore.
LGTM. Good work on refining that library. |
2aed87e
to
5b2769a
Compare
Thanks @MichaelZapataScality. I'm currently adding some more changes to this PR so @AntoninCoulibaly and I can write unit tests on sane basis. |
5c43b2a
to
840eb58
Compare
👍 Nice refactor ;) |
Ready for review from you @MichaelZapataScality 🔎 ;-) |
response.setChunk(new Buffer('value')); | ||
|
||
const sock = net.connect(1234, 'localhost'); | ||
const ret = response.send(sock); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
response.send(sock) ? I might be too used to the sock.send(response) architecture, but it looks strange to me, in OOP terms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not change this. This PR is a refactor only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, might be for another round then :)
Despite all my remarks that are not completely relevant to this PR (due to the fact that I've never reviewed any code from Kinetic PRs until now), I'm ok with this PR, though I think it will require a cleanup soon enough, for which I'm ready to make myself available to discuss the different details I commented here. |
840eb58
to
cbb6a28
Compare
@DavidPineauScality Thanks for your comments, I addressed them. Everything should be fine now. |
Big refactor of the Kinetic library: - move constants out of instances (memory footprint) - give a proper name to instances (`Kinetic.PDU`) - modify tests consequently - document library usage in README.md import Kinetic from 'Kinetic'; const rawData = new Buffer('\x46\x00\x00\x00\x32\x00'); const kineticPDU = new Kinetic.PDU(rawData); const sock = net.connect(1234, 'localhost'); const ret = kineticPDU.send(sock); if (ret !== Kinetic.errors.SUCCESS) throw new Error("argh: " + Kinetic.getErrorName(ret) + "!"); Fixes: #18
The Node.js standard `Buffer.equals()` already exists for that.
- Add a `_computeHMAC` function to avoid code duplication. - Have consistent return type in `hmacIntegrity` instead of returning either `true` or `errors.HMAC_FAILURE`.
Currently to create a "PUT" Kinetic PDU, one must run: const pdu = new Kinetic.PDU(); pdu.put(); This is not object-oriented programming, and is error-prone since we don't expect commands like `put`, `flush` etc. to create messages with a given type. This patch creates classes that inherit from `PDU`, e.g. `PutPDU`, `GetPDU`, `GetLogPDU` etc. and that can be used the same way.
Remove an unneeded test that was provoking errors.HMAC when parsing Kinetic PDUs.
cbb6a28
to
b8e799d
Compare
👍 |
Refactor Kinetic lib for static constants and PDU class
Big refactor of the Kinetic library:
Kinetic.PDU
)Fixes: #18
Kinetic: Define PDU commands as classes
Currently to create a "PUT" Kinetic PDU, one must run:
This is not object-oriented programming, and is error-prone since we don't expect commands like
put
,flush
etc. to create messages with a given type.This patch creates classes that inherit from
PDU
, e.g.PutPDU
,GetPDU
,GetLogPDU
etc. and that can be used the same way.Sample from README: