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

"With context" support (for use with FlowWithContext) #780

Merged
merged 27 commits into from
Jun 20, 2019

Conversation

ennru
Copy link
Member

@ennru ennru commented Apr 30, 2019

Purpose

This adds Producer.withContext to support Akka's new FlowWithContext out of the box.
It works the same as flexiFlow with the difference that the PassThrough is unavailable to the user as it is used to carry the context along internally.

Adds Consumer.sourceWithOffsetContext which creates SourceWithContext[ConsumerRecord[K, V], CommittableOffset, Control].

References

Source.asSourceWithContext

Flow.asFlowWithContext

Changes

  • Use Akka 2.5.23 which improves new operators
  • Add a method to Envelope to set the pass-through value

Producer

  • Add flowWithContext methods

Consumer

  • Add sourceWithOffsetContext

Transactional

  • Add sourceWithOffsetContext
  • Add sinkWithOffsetContext
  • Add flowWithOffsetContext

Committer

  • Add flowWithOffsetContext
  • Add sinkWithOffsetContext

@ennru
Copy link
Member Author

ennru commented Apr 30, 2019

cc @RayRoestenburg

@ennru
Copy link
Member Author

ennru commented Apr 30, 2019

The implementation of the Java Consumer can be improved if akka/akka#26836 gets fixed.

Copy link
Member Author

@ennru ennru left a comment

Choose a reason for hiding this comment

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

The Java DSL is not stable, yet.

@ennru
Copy link
Member Author

ennru commented May 23, 2019

@RayRoestenburg Would be great if you'd have a look at this.

@RayRoestenburg
Copy link

@ennru sorry I got my mail filters setup wrong somehow, will have a look.

Copy link

@RayRoestenburg RayRoestenburg left a comment

Choose a reason for hiding this comment

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

Super minor comments, LGTM

@ennru ennru added this to the 1.0.4 milestone May 31, 2019
Copy link
Member

@2m 2m left a comment

Choose a reason for hiding this comment

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

Some questions regarding API naming.

@ennru ennru removed this from the 1.0.4 milestone Jun 10, 2019
Copy link
Member

@2m 2m left a comment

Choose a reason for hiding this comment

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

Couple of more comments.

core/src/main/scala/akka/kafka/scaladsl/Committer.scala Outdated Show resolved Hide resolved
core/src/main/scala/akka/kafka/scaladsl/Committer.scala Outdated Show resolved Hide resolved
* Batches offsets from context and commits them to Kafka.
*/
@ApiMayChange
def sinkWithOffsetContext[E](settings: CommitterSettings): Sink[(E, Committable), Future[Done]] =
Copy link
Member

Choose a reason for hiding this comment

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

We still need to fix the name here. sinkWithCommitableContext? Same for the flow above?

Copy link
Member Author

Choose a reason for hiding this comment

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

But it is the same as the flow?! withOffsetContext

Copy link
Member

Choose a reason for hiding this comment

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

Yes. I think that one should also be renamed to flowWithCommittableContext as that is what the context type is.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a hard one. The API is actually lying, it can't handle "any" Committable as everything but offsets will fail here

https://github.com/akka/alpakka-kafka/blob/master/core/src/main/scala/akka/kafka/internal/MessageBuilder.scala#L131-L134

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed those new withOffsetContext methods to require CommittableOffset other Committables won't do anyway.

/**
* Batches offsets and commits them to Kafka, emits `CommittableOffsetBatch` for every committed batch.
*/
def batchFlow[C <: Committable](settings: CommitterSettings): Flow[C, CommittableOffsetBatch, NotUsed] =
Copy link
Member Author

Choose a reason for hiding this comment

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

This was lacking in the Java DSL, I'm tempted to require CommittableOffset here as well, but that would differ from the Scala DSL.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think it is good to keep them the same.

Copy link
Member

@2m 2m left a comment

Choose a reason for hiding this comment

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

LGTM. After the small fix in docs, this can be merged in.

| Factory method | Stream element type | Emits |
|-------------------------|--------------------------------|--------------|
| `sink` | `Committable` | N/A |
| `sinkWithOffsetContext` | Any (`Committable` in context) | N/A |
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
| `sinkWithOffsetContext` | Any (`Committable` in context) | N/A |
| `sinkWithOffsetContext` | Any (`CommittableOffset` in context) | N/A |

| `sinkWithOffsetContext` | Any (`Committable` in context) | N/A |
| `flow` | `Committable` | `Done` |
| `batchFlow` | `Committable` | `CommittableOffsetBatch` |
| `flowWithOffsetContext` | Any (`Committable` in context) | `NotUsed` (`CommittableOffsetBatch` in context) |
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
| `flowWithOffsetContext` | Any (`Committable` in context) | `NotUsed` (`CommittableOffsetBatch` in context) |
| `flowWithOffsetContext` | Any (`CommittableOffset` in context) | `NotUsed` (`CommittableOffsetBatch` in context) |

@ennru ennru added this to the 1.0.5 milestone Jun 20, 2019
@ennru ennru merged commit 33038bb into akka:master Jun 20, 2019
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.

3 participants