snowplow-incubator / sauna

:hotsprings: A decisioning and response platform
https://github.com/snowplow/sauna/wiki
69 stars 11 forks source link

Sauna 0.2.0 #74

Closed rzats closed 7 years ago

rzats commented 7 years ago

Contains

Outstanding before merging

alexanderdean commented 7 years ago

Thanks @rzats - can you talk to @chuwy please about the Iglu Scala Client...

alexanderdean commented 7 years ago

Removing @jbeemster as a reviewer as he is on holiday

alexanderdean commented 7 years ago

@chuwy can you do the first pass review please?

chuwy commented 7 years ago

@alexanderdean yes, will do.

chuwy commented 7 years ago

I'm also sorry that you have to work with this Iglu/play-json combination as most of functionality you need was already implemented in iglu core, but for json4s and circe libraries. We never used play json before.

alexanderdean commented 7 years ago

I'm also sorry that you have to work with this Iglu/play-json combination as most of functionality you need was already implemented in iglu core, but for json4s and circe libraries. We never used play json before.

What's the reason for using Play JSON?

chuwy commented 7 years ago

@alexanderdean it was picked by intern who started to write sauna, no other reasons.

rzats commented 7 years ago

@alexanderdean @chuwy: this is ready for another round of reviews.

A few notes:

alexanderdean commented 7 years ago

Thanks - @chuwy can you do another pass?

rzats commented 7 years ago

@chuwy: I've separated the function into two for now. Still not an ideal solution (the same amount of match blocks and LOC is the same), but it's not as nested - and allows us to programmatically generate Commands for tests etc.

chuwy commented 7 years ago

Hey @rzats,

I just pushed my proposal for #75 in https://github.com/snowplow/sauna/commit/b3c434969a64189949fdf16bcf7391e8b66625f6 (I didn't run tests, but it should be trivial to modify them).

I need to admit that parametrizing all events in 0.1.0 was a slight over-engineering. Type parameters there don't add any typesafety and don't generalize anything. Designer of responder always knows what exactly observer events he can parse into responder events, so type parameter here don't make much sense, usual subtyping works fine. This all becomes even more important taking in account akka design, erasure, etc.

rzats commented 7 years ago

@alexanderdean @chuwy: I've now implemented all the features from the 0.2.0 milestone. This should be ready for a final review!

alexanderdean commented 7 years ago

Awesome - thanks @rzats! Requesting the first pass review from Anton...

rzats commented 7 years ago

@chuwy: fixed most of the issues you've mentioned (besides command documentation - still need some clarification from @alexanderdean on what some of the metadata actually means). Also GitHub doesn't let me reply to your last comment, but yes, all the new environment variables are on Travis.

chuwy commented 7 years ago

Well done, @rzats! I also made another pass on Kinesis Observer and it looks fine.

Two last things:

rzats commented 7 years ago

@chuwy: done!

alexanderdean commented 7 years ago

Closing, will open a PR with the new branch.