-
Notifications
You must be signed in to change notification settings - Fork 31
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
Release 2.2.1 #124
Release 2.2.1 #124
Conversation
eee0183
to
bd5bff8
Compare
Hey @dilyand , could you link this PR to an issue, so that we can get some context about what we try to achieve please ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the most part I think this is a good change. It is clear and easy to understand even though it is implementing quite a complicated retry algorithm. I just made a few comments.
...src/main/scala/com.snowplowanalytics.snowplow.collectors.scalastream/sinks/KinesisSink.scala
Show resolved
Hide resolved
...src/main/scala/com.snowplowanalytics.snowplow.collectors.scalastream/sinks/KinesisSink.scala
Show resolved
Hide resolved
...src/main/scala/com.snowplowanalytics.snowplow.collectors.scalastream/sinks/KinesisSink.scala
Outdated
Show resolved
Hide resolved
...src/main/scala/com.snowplowanalytics.snowplow.collectors.scalastream/sinks/KinesisSink.scala
Outdated
Show resolved
Hide resolved
...src/main/scala/com.snowplowanalytics.snowplow.collectors.scalastream/sinks/KinesisSink.scala
Show resolved
Hide resolved
...src/main/scala/com.snowplowanalytics.snowplow.collectors.scalastream/sinks/KinesisSink.scala
Show resolved
Hide resolved
bd5bff8
to
0d48512
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much better than what we had before @dilyand !
It would be great to split KinesisSink
into smaller files, e.g.
KinesisSink
with the initialization and retry logicKinesisEmitter
with everything related to writing to KinesisSqsEmitter
with everything related to writing to SQSEventStorage
with the buffer
Did you manage to test the switching mechanism ?
Not related to this PR but now that we have the split
function it would be great to add it to SQS collector, so that we don't rely on humans always configuring the limits right.
...src/main/scala/com.snowplowanalytics.snowplow.collectors.scalastream/sinks/KinesisSink.scala
Show resolved
Hide resolved
...src/main/scala/com.snowplowanalytics.snowplow.collectors.scalastream/sinks/KinesisSink.scala
Show resolved
Hide resolved
...src/main/scala/com.snowplowanalytics.snowplow.collectors.scalastream/sinks/KinesisSink.scala
Show resolved
Hide resolved
...src/main/scala/com.snowplowanalytics.snowplow.collectors.scalastream/sinks/KinesisSink.scala
Outdated
Show resolved
Hide resolved
...src/main/scala/com.snowplowanalytics.snowplow.collectors.scalastream/sinks/KinesisSink.scala
Outdated
Show resolved
Hide resolved
...src/main/scala/com.snowplowanalytics.snowplow.collectors.scalastream/sinks/KinesisSink.scala
Show resolved
Hide resolved
...src/main/scala/com.snowplowanalytics.snowplow.collectors.scalastream/sinks/KinesisSink.scala
Show resolved
Hide resolved
...src/main/scala/com.snowplowanalytics.snowplow.collectors.scalastream/sinks/KinesisSink.scala
Outdated
Show resolved
Hide resolved
...src/main/scala/com.snowplowanalytics.snowplow.collectors.scalastream/sinks/KinesisSink.scala
Show resolved
Hide resolved
...src/main/scala/com.snowplowanalytics.snowplow.collectors.scalastream/sinks/KinesisSink.scala
Show resolved
Hide resolved
Anonymous mode not enabled:1.) No nuid in qs or in cookie:
2.) Nuid in qs:
3.) Nuid in cookie:
4.) Nuid in qs and cookie:
Anonymous mode enabled:1.) No nuid in qs or in cookie:
2.) Nuid in qs:
3.) Nuid in cookie:
4.) Nuid in qs and cookie:
|
3877728
to
2a7f767
Compare
…or_payload_format_violation (close #126)
2a7f767
to
4e58556
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:)
4e58556
to
b253b68
Compare
The logging in
sinkBatch
can potentially be extracted to make the function a bit less busy. Looking forward to opinions on that.