Skip to content
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

Put record options #2

Closed
wants to merge 4 commits into from
Closed

Conversation

Philmod
Copy link

@Philmod Philmod commented Feb 4, 2014

  • options to set ExplicitHashKey and SequenceNumberForOrdering
  • emit the shardId/sequenceNumber with the data

@mhart
Copy link
Owner

mhart commented Feb 4, 2014

Cool! Just looking through it now

@@ -134,7 +134,7 @@ function getRecords(data, cb) {
for (i = 0; i < res.Records.length; i++) {
record = res.Records[i]
self.shardIds[data.ShardId].lastSequenceNumber = record.SequenceNumber
self.emit('data', new Buffer(record.Data, 'base64'))
self.emit('data', new Buffer(record.Data, 'base64'), {shardId: data.ShardId, sequenceNumber: record.SequenceNumber})
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, this is not how data emitting or streams usually work. The data event should just have one parameter.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to know which sequence/shardId is linked to the data, not in another event.
Otherwise we can construct a Object there:

self.emit('data', {
  data: new Buffer(record.Data, 'base64'),
  shardId: data.ShardId, 
  sequenceNumber: record.SequenceNumber
});

?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case we'd need to support two modes for the stream - one being objectMode and one just normal buffer stream mode (you can't emit objects unless the stream is in objectMode)

We could have an option passed in to the constructor for this.

mhart added a commit that referenced this pull request Feb 14, 2014
…jectMode

Thanks to @Philmod for the suggestions and initial code contribution in
#2
@mhart
Copy link
Owner

mhart commented Feb 14, 2014

Resolved in 9375e2b and released as 0.2.0 - thanks for the contribution!

(So you'll probably want to use objectMode to achieve what you'd like)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants