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

Added a source that doesn't automatically ACK messages to rabbit #291 #292

Closed
wants to merge 2 commits into from

Conversation

Falmarri
Copy link
Contributor

@Falmarri Falmarri commented May 9, 2017

There's some parts of this that I'm not sure about. Specifically the implementation of Ack and Nack being subclasses of an arbitrary Command function.

@johanandren
Copy link
Member

Refs #97

Copy link

@ndriscoll ndriscoll left a comment

Choose a reason for hiding this comment

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

I think there should be a way to parameterize the AmqpSourceStage on the message type (already Acked or not) to try to cut down on the duplication. Will think about it more.

FWIW, if it wouldn't break other things, I think AckedIncomingMessage and IncomingMessage are clearer than IncomingMessage and IncomingMessageWithAck, respectively (like instead of saying you can ack it, reverse it and say what is right now IncomingMessage has already been Acked)

/**
* Connects to an AMQP server upon materialization and consumes messages from it emitting them
* into the stream. Each materialized source will create one connection to the broker.
* As soon as an `IncomingMessage` is sent downstream, an ack for it is sent to the broker.

Choose a reason for hiding this comment

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

Need to fix comment

build.sbt Outdated
.settings(
name := "akka-stream-alpakka-amqp",
version := "0.5.3-sigfig",

Choose a reason for hiding this comment

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

Whoops :)

Changes Amqp sinks to materialize to Future[Done]. As currently it was
very difficult to determine when/if a sink failed due to a amqp error.
@johanandren
Copy link
Member

Closing this as it was superseded by #483 that just got merged.

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.

None yet

3 participants