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

Publish with confirm listener added #56

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

sksingh-55
Copy link
Contributor

build failure fix
publish with confirm listener function added
TestCase for the publishWithConfirmListener

publish with confirm listener function added
TestCase for the publishWithConfirmListener
stdout Outdated
@@ -0,0 +1,266 @@
[INFO] Scanning for projects...
Copy link

Choose a reason for hiding this comment

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

Why is this file needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removing this added by mistake

@@ -35,8 +35,8 @@
@Slf4j
public class NamespacedQueuesTest {

private static final int RABBITMQ_MANAGEMENT_PORT = 15672;
private static final String RABBITMQ_DOCKER_IMAGE = "rabbitmq:3-management";
private static final int RABBITMQ_MANAGEMENT_PORT = 5672;
Copy link

Choose a reason for hiding this comment

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

Any reason to change the port?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

15672 is for browser

public void start() throws Exception {
final String exchange = config.getExchange();
final String dlx = config.getExchange() + "_SIDELINE";
this.publishChannel = connection.newChannel();
Copy link

Choose a reason for hiding this comment

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

WHy is this change done?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverting this change

@@ -106,17 +114,105 @@ public final long pendingSidelineMessagesCount() {
return Long.MAX_VALUE;
}

/**
* Note: Timeout is in MilliSeconds, Function take a list of message as input and return not ack msg as output
Copy link

Choose a reason for hiding this comment

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

We are taking timeunit, so maybe this comment needs to change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

publishChannel.addConfirmListener((sequenceNumber, multiple) -> {
messagesAck(sequenceNumber, multiple, outstandingConfirms, publishAckLatch);
}, (sequenceNumber, multiple) -> {
nackedMessages.addAll(messagesNack(sequenceNumber, multiple, outstandingConfirms, publishAckLatch));
Copy link

Choose a reason for hiding this comment

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

Why not just assign the return value rather than this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because there can be multiple callback with multiple = true

Copy link

Choose a reason for hiding this comment

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

Ah, my bad

}
}

if (!publishAckLatch.await(unit.toMillis(timeout), TimeUnit.MILLISECONDS)) {
Copy link

Choose a reason for hiding this comment

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

What is wrong with publishAckLatch.await(timeout, unit)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


long endTime = System.nanoTime();

log.info(String.format("Published %d messages with confirmListener in %d ms", messages.size() - outstandingConfirms.size(),
Copy link

Choose a reason for hiding this comment

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

We won't know how many messages were submitted, i.e. total messages.

@@ -24,7 +32,7 @@
private final ObjectMapper mapper;
private final String queueName;

private Channel publishChannel;
public Channel publishChannel;
Copy link

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted it, created a function to set publish channel

@@ -6,7 +6,7 @@

<groupId>io.appform.dropwizard.actors</groupId>
<artifactId>dropwizard-rabbitmq-actors</artifactId>
<version>2.0.28-1</version>
<version>3.0.2_Test</version>

Choose a reason for hiding this comment

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

why changing versions manually

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