Page MenuHomePhabricator

Commit for review
ClosedPublic

Authored by Ottomata on Oct 19 2016, 9:43 PM.

Details

Reviewers
Pchelolo
Ottomata
Commits
rWKSEcd8aa5cdf923: Commit for review
Patch without arc
git checkout -b D419 && curl -L https://phabricator.wikimedia.org/D419?download=true | git apply
Summary

Bug: T148470

Diff Detail

Repository
rWKSE KafkaSSE
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Ottomata updated this revision to Diff 1116.Oct 19 2016, 9:43 PM
Ottomata retitled this revision from to Commit for review.
Ottomata updated this object.
Ottomata edited the test plan for this revision. (Show Details)
Ottomata added a reviewer: Pchelolo.
Pchelolo edited edge metadata.Oct 19 2016, 11:39 PM

I didn't look too close, but LGTM overall

index.js
24

no need for a dot

lib/KafkaSSE.js
18

This module has native bindings, better use cassandra-uuid module. The less native bindings we use the better.

268

I don't see a point in creating a promise here. You could do this sync and then start promise chain with createKafkaConsumer.

Ottomata added inline comments.Oct 20 2016, 2:01 PM
lib/KafkaSSE.js
268

Yeah, I only did this so I could validate last-event-id as soon as possible. I wanted to do this before I created the KafkaConsumer, as creating KafkaConsumer has overhead and side affects, and I'd rather fail early if we know that we will.

I agree that we don't need a Promise for this, but I wanted to take advantage of the .catch error handing in connect() above. If _init doesn't return a Promise or throw from in a Promise, the .catch won't work (will it?).

Ottomata added inline comments.Oct 20 2016, 2:06 PM
lib/KafkaSSE.js
18

I see that express-request-id uses node-uuid. Can I use that?

Or, cassandra-uuid is fine, it just sounds so 'cassandra' specific (even though I can see it isn't). You'd recommend cassandra-uuid over node-uuid? If so I'll use it.

I didn't look too close, but LGTM overall

lib/KafkaSSE.js
18

@Ottomata cassandra-uuid is called like that because the implementation was copy-pasted from cassandra driver. We use it everywhere, but you can use either, just not the one with native bindings.

268

Oh, sorry, disregard, I didn't notice it's not called from within a promise.

Ottomata updated this revision to Diff 1118.Oct 20 2016, 6:57 PM
Ottomata marked 2 inline comments as done.
Ottomata edited edge metadata.
  • Use node-uuid instead of uuid to avoid native dependency
Ottomata accepted this revision.Oct 20 2016, 6:57 PM
Ottomata added a reviewer: Ottomata.
This revision is now accepted and ready to land.Oct 20 2016, 6:57 PM
This revision was automatically updated to reflect the committed changes.