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

service/firehose: Consider using interface for Firehose Record #546

Closed
harlow opened this issue Feb 8, 2016 · 3 comments
Closed

service/firehose: Consider using interface for Firehose Record #546

harlow opened this issue Feb 8, 2016 · 3 comments
Assignees
Labels
feature-request A feature should be added or improved.

Comments

@harlow
Copy link

harlow commented Feb 8, 2016

Params for firehose.PutRecordBatchInput and firehose.PutRecordInput currently require a struct of type firehose.Record.

It would be great if was defined with an interface Record with Data field. This way any struct that meets this contract (i.e. kinesis.Record) could also be used (instead of having to copy data into firehose.Record structs)

Thoughts?

// The unit of data in a delivery stream.
type Record struct {
    _ struct{} `type:"structure"`

    // The data blob, which is base64-encoded when the blob is serialized. The maximum
    // size of the data blob, before base64-encoding, is 1,000 KB.
    Data []byte `type:"blob" required:"true"`
}

// The unit of data in a delivery stream.
type Record struct {
_ struct{} `type:"structure"`
// The data blob, which is base64-encoded when the blob is serialized. The maximum
// size of the data blob, before base64-encoding, is 1,000 KB.
Data []byte `type:"blob" required:"true"`
}

@nightlyone
Copy link

That sounds like a great idea to me, too. Data could be replaced that way and the firehose.PutXX functions could pass in a pre-allocated buffer to a method Data(w io.Writer) which can be re-used.

@xibz xibz added the feature-request A feature should be added or improved. label Feb 8, 2016
@xibz xibz self-assigned this Feb 8, 2016
@xibz
Copy link
Contributor

xibz commented Feb 8, 2016

Hello @harlow and @nightlyone, thank you for suggesting this! I am tagging this as a feature request. We are more than happy to look at PRs. Unfortunately, we cannot mutate the Record object directly, as this would be a breaking change. However, We can add another layer on top of the service api. This layer would have some sort of interface. We can then call an interface method to adapt the interface to the Record object. If you have any further questions, let us know.

@jasdel jasdel changed the title Consider using interface for Firehose Record service/firehose: Consider using interface for Firehose Record Apr 12, 2017
@jasdel
Copy link
Contributor

jasdel commented Nov 9, 2017

Going to close this feature request as this change would require a breaking change to the SDK to change the Record type to an interface. In addition this would prevent the SDK being forward compatible with the service if the service were to add additional optional fields to the Record type as the interface would not be able to represent them.

This logic could better exist as a higher level abstraction on top of the Kinesis service.

@jasdel jasdel closed this as completed Nov 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved.
Projects
None yet
Development

No branches or pull requests

4 participants