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

NonDurable cursor for managed ledger #366

Merged
merged 1 commit into from
May 6, 2017

Conversation

merlimat
Copy link
Contributor

Motivation

As part of #355, ManagedLedger needs to support a lightweight cursor that can be just used for reading from a specific initial position.

Modifications

Introduce new ML API to create a non-durable cursor. This cursor is exactly the same as a regular cursor except in that it doesn't persist the mark-delete position (or the individually deleted messages). Also, it doesn't prevent entries to be deleted.

Result

Can implement topic reader by plugging a non-durable cursor into a regular broker subscription and dispatcher objects.

@merlimat merlimat added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label Apr 19, 2017
@merlimat merlimat added this to the 1.18 milestone Apr 19, 2017
@merlimat merlimat self-assigned this Apr 19, 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 comment and one question

@@ -135,6 +135,23 @@
public ManagedCursor openCursor(String name) throws InterruptedException, ManagedLedgerException;

/**
* Creates a new cursor whose metadata is not backed by durable storage. A caller can treat the non-durable cursor
* exactly like a normal cursor, with the only difference in that after restart it will not remember which entries
* were deleted. Also it does not prevent data from being deleted.
Copy link
Contributor

@rdhabalia rdhabalia Apr 22, 2017

Choose a reason for hiding this comment

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

Also it does not prevent data from being deleted.

Isn't it create unexpected behavior for client when slowest durable cursor moves to next ledger and ml trims the ledger. Let's say if this cursor is slower than other durable cursor then ManagedLedger will immediately delete data once durable-cursor consumeAndAck from that ml-ledger and switch the ledger, so non-durable cursor may not get expected chunk of data in stream.

  • Instead can't we treat non-durable cursor like durable and not delete until it consume messages. Because if non-durable cursor/consumer disconnects then broker is anyway removes this non-durable cursor and can move position according to durable-cursor's markDeletePosition.

public class PositionImpl implements Position, Comparable<PositionImpl> {

private final long ledgerId;
private final long entryId;

public static Position earliest = new PositionImpl(-1, -1);
public static Position latest = new PositionImpl(Long.MAX_VALUE, Long.MAX_VALUE);
Copy link
Contributor

Choose a reason for hiding this comment

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

MIN_POSITION / MAX_POSITION?

checkManagedLedgerIsOpen();
checkFenced();

return new NonDurableCursorImpl(bookKeeper, config, this, null, (PositionImpl) startCursorPosition);
Copy link
Contributor

Choose a reason for hiding this comment

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

for debugging, should broker generate or expect cursor-name or id rather passing null?

@merlimat
Copy link
Contributor Author

@rdhabalia Maybe the name NonDurableCursor doesn't fully convey the intended semantic for the new class. The context here is just to have a way to read through a topic (eg: support TopicReader) and reuse as much code as possible from the regular cursor implementation. Basically all the cache code and the logic for how to switch to "next valid position", plus the asyncReadOrWait stuff.

About non impeding messages to be deleted, consider that in case of non-durable topic, during a disconnection the cursor will go away and data will get potentially deleted anyway. So I prefer it to be explicitly ingrained into the API. Same thing about naming the cursor. It will go away in any case after a restart, so I don't see the advantage of naming it.

When data gets deleted, the cursor will just skip over it. The intended usage for the non-durable cursor and topic reader is in conjunction with the message retention, to make sure data sticks around for the intended amount of time

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 merlimat merged commit 65da593 into apache:master May 6, 2017
hrsakai pushed a commit to hrsakai/pulsar that referenced this pull request Dec 10, 2020
Fixes apache#366

### Motivation

In current code of `pulsar/internal/connection.go` we have 2 channels, closeCh and incomingRequestsCh. when the connection closes, the current mis-use of these 2 channels may have a deadlock. 
PR apache#366 has detailed steps to reproduce and the root cause [analysis](apache/pulsar-client-go#366 (comment)) .
This PR tries to fix the deadlock.

### Modifications
- make the close logic independent, not in the same loop of normal events handling.
- when the connection closed, handle the existing requests in the channel and return an error to avoid deadlock.

### Verifying this change
passed the tests in apache#366 
current ut passed
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.

None yet

2 participants