-
Notifications
You must be signed in to change notification settings - Fork 647
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
Jms passthrough envelope #1212
Jms passthrough envelope #1212
Conversation
Compilation errors, did you push all changes? |
Scala 2.11 bit me again, still not in my natural developer flow to double-check with older Scala versions. Sorry for that, I will have a look tonight. |
No worries. That's what we have Travis for, correct? |
Yes, agreed 👍 |
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.
LGTM.
@akara Anything you'd like to suggest?
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.
Just a few comments on documentation and test. The implementation makes sense and LGTM.
.runWith(Sink.seq(), materializer); | ||
// #run-flexi-flow-pass-through-producer | ||
|
||
assertEquals(data, result.toCompletableFuture().get()); |
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.
Do we need to assert that none of these pass-through envelopes are published?
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.
Sure, I'll add an assertion for that
val result = Source(in).via(jmsProducer).map(_.passThrough).runWith(Sink.seq) | ||
//#run-flexi-flow-pass-through-producer | ||
|
||
result.futureValue shouldEqual data |
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.
I'd prefer to mock this one instead so you can actually collect the messages that are published vs the messages that are not published. You can achieve the same with creating a consumer to validate your messages as well.
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.
You mean mocking the connection factory (and session etc) and asserting that send
was never invoked?
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.
That's what I'm thinking. Thanks!
final case class Message[+M <: JmsMessage, +PassThrough](message: M, passThrough: PassThrough) | ||
extends Envelope[M, PassThrough] | ||
|
||
final case class PassThroughMessage[+M <: JmsMessage, +PassThrough](passThrough: PassThrough) |
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.
We may want to document the use case for PassThroughMessage
similar to the Scaladocs in the Kafka case, or document it in the main documentation. Otherwise it may not be clear when and why to use this class.
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.
Good point, I'll add a section in the docs explaining that this allows to ack messages consumed without producing, and without the need for fan-out flows.
- Adjusted the design even more to Alpakka Kafka - Renamed JmsProducerEnvelope to JmsProducerMessage - Moved Envelope to inner trait of JmsProducerMessage
- extended passthrough tests - extended documentation of passthrough messages
b74033a
to
9911170
Compare
Addressed reviewer comments and rebased on master |
Thank you, the JMS connector is getting quite an overhaul! |
Fixes #1211