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

Position Reader on a specific message within a batch #720

Merged
merged 4 commits into from
Sep 6, 2017

Conversation

merlimat
Copy link
Contributor

Motivation

Allow the Topic reader to position on a particular message within a batch.

@merlimat merlimat added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label Aug 29, 2017
@merlimat merlimat added this to the 1.20.0-incubating milestone Aug 29, 2017
@merlimat merlimat self-assigned this Aug 29, 2017
@merlimat merlimat requested a review from rdhabalia August 29, 2017 01:50
@merlimat merlimat requested review from jai1 and saandrews August 29, 2017 02:01
@merlimat
Copy link
Contributor Author

retest this please

&& messageId.getEntryId() == startMessageId.getEntryId()
&& i <= startMessageId.getBatchIndex()) {
// If we are receiving a batch message, we need to discard messages that were prior
// to the startMessageId
Copy link
Contributor

Choose a reason for hiding this comment

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

just a quick suggestion if that make sense.

  1. what if we also add similar validation at messageReceived() for non-batch-msg also where we skip the message if receivedMsgId < startMessageId
  2. and at broker we always read from entryId=entryId-1 and duplicate entry will be anyway skipped by the client.

In this way, broker can be BatchMessage agnostic and it will not have any batch-msg specific logic as it treats batch-msg as normal msg only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that checking everything in client side gets a bit more complicated for the comparison. Also we'd have to do it in 2 places one for batch messages and one for regular. Plus in C++ and Java.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that checking everything in client side gets a bit more complicated for the comparison.

Oh. Other than adding skip-condition for non-batch msg, do we have to do anything additionally? My take would be, handling duplicate msg at client (because anyway we are doing for batch-msg so, why not for normal msg) is better than adding small-hack at broker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uhm, I still prefer doing it in server side :)

What about encapsulating in the MessageIdImpl so that we can have a method to get the "right" message id and that method can be overriden in the BatchMessageIdImpl class?

Copy link
Contributor

Choose a reason for hiding this comment

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

Uhm, I still prefer doing it in server side :)

Alright.. 👍

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.

👍

@merlimat
Copy link
Contributor Author

merlimat commented Sep 5, 2017

retest this please

@merlimat merlimat force-pushed the java-reader-batches branch from 672c044 to f70c868 Compare September 5, 2017 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants