Bug: T148470
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
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
I didn't look too close, but LGTM overall
index.js | ||
---|---|---|
23 | no need for a dot | |
lib/KafkaSSE.js | ||
17 | This module has native bindings, better use cassandra-uuid module. The less native bindings we use the better. | |
267 | I don't see a point in creating a promise here. You could do this sync and then start promise chain with createKafkaConsumer. |
lib/KafkaSSE.js | ||
---|---|---|
267 | 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?). |
lib/KafkaSSE.js | ||
---|---|---|
17 | 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 | ||
---|---|---|
17 | @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. | |
267 | Oh, sorry, disregard, I didn't notice it's not called from within a promise. |