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

Scala Common Enrich: etl_tstamp in Enrichment Manager should be a Joda DateTime not a String #1841

Closed
jbeemster opened this issue Jul 2, 2015 · 6 comments
Assignees

Comments

@jbeemster
Copy link
Member

No description provided.

@jbeemster jbeemster self-assigned this Jul 2, 2015
@jbeemster jbeemster added this to the Release 68 Bird TBC milestone Jul 2, 2015
@alexanderdean alexanderdean modified the milestones: Release 69 Bird TBC, Release 68 Bird TBC Jul 2, 2015
@alexanderdean
Copy link
Member

Pushing back to R69

@alexanderdean
Copy link
Member

This will ensure better type safety...

@alexanderdean alexanderdean assigned fblundun and unassigned jbeemster Jul 27, 2015
@fblundun
Copy link
Contributor

The nice thing about keeping this a String rather than a DateTime in the EnrichmentManager is that it means we don't need to convert the DateTime to a Redshift-compatible string with every event we process.

Instead we could make EtlPipeline a class rather than an object and give it a constructor with a DateTime etlTstamp parameter.

@alexanderdean
Copy link
Member

Hmm - how would the EtlPipeline as a class sit with Kinesis Enrich? Given that the output for Common Enrich won't be Redshift-formatted in a few releases' time anyway, I'm not too stressed about the conversion (in other words, soon we will have lots of conversions anyway).

@fblundun
Copy link
Contributor

With Kinesis Enrich you would create a new EtlPipeline with a new timestamp (or update the timestamp of the existing EtlPipeline) for every call to GetRecords.

But proportionally, the effect on performance should be pretty small, so maybe we just make it a DateTime and do the conversion in each call to enrichEvent.

@alexanderdean
Copy link
Member

Thanks for clarifying - yep - I think let's just make it a DateTime...

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

No branches or pull requests

3 participants