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

Allow consumer to seek to message id from within Pulsar client #848

Merged
merged 2 commits into from
Oct 24, 2017

Conversation

merlimat
Copy link
Contributor

Motivation

Currently we have the option to "reset" a subscription to a specific timestamp or message id through the Pulsar admin API.

In some cases, it might be useful to allow the same operation to be done from the Consumer API. One such example is related to the Pulsar-Kafka wrapper, in which we need to emulate operations like seekToBeginning(), seekToEnd() or seekTo().

Modifications

Added new request command in wire protocol to issue a "seek" command to the broker, relative to the subscription associated with the current consumer.

@merlimat merlimat added the type/feature The PR added a new feature or issue requested a new feature label Oct 23, 2017
@merlimat merlimat added this to the 1.21.0-incubating milestone Oct 23, 2017
@merlimat merlimat self-assigned this Oct 23, 2017
Copy link
Contributor

@rdhabalia rdhabalia left a comment

Choose a reason for hiding this comment

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

LGTM.. just minor comments.

@@ -1910,6 +1910,7 @@ boolean isValidPosition(PositionImpl position) {
log.debug("IsValid position: {} -- last: {}", position, last);
}

// if (position.equals(PositionImpl.earliest || position.equals(PositionImpl)))
Copy link
Contributor

Choose a reason for hiding this comment

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

can we remove this comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ups, forgot those in there

Consumer consumer = consumerFuture.getNow(null);
Subscription subscription = consumer.getSubscription();
MessageIdData msgIdData = seek.getMessageId();
// MessageIdImpl msgId = MessageIdImpl.earliest
Copy link
Contributor

Choose a reason for hiding this comment

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

can we remove this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same, will cleanup

try {
seekAsync(messageId).get();
} catch (ExecutionException | InterruptedException e) {
throw new PulsarClientException(e);
Copy link
Contributor

Choose a reason for hiding this comment

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

e.getCause()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it's interrupted we might not have the "cause". Though I can refactor and have 2 sections here.

}).exceptionally(ex -> {
log.warn("[{}][{}] Failed to reset subscription: {}", remoteAddress, subscription, ex.getMessage(), ex);
ctx.writeAndFlush(Commands.newError(seek.getRequestId(), ServerError.UnknownError,
"Error when resetting subscription: " + ex.getMessage()));
Copy link
Contributor

Choose a reason for hiding this comment

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

ex.getCause().getMessage()?

Copy link
Contributor

@rdhabalia rdhabalia left a comment

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/feature The PR added a new feature or issue requested a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants