[improve][pip] PIP-282: Change definition of the recently joined consumers position #20776

equanz · 2023-07-11T04:48:22Z

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

Documentation

doc
doc-required
doc-not-needed
doc-complete

pip/pip-282.md

equanz · 2023-07-31T01:34:14Z

@poorbarcode Sorry for the late reply. I addressed your comments. PTAL

poorbarcode · 2023-08-28T16:59:40Z

pip/pip-282.md

+     private final LinkedHashMap<Consumer, PositionImpl> recentlyJoinedConsumers;
+
+    private PositionImpl lastSentPosition;
+    private final RangeSetWrapper<PositionImpl> individuallySentPositions;


individuallySentPositions = {Range-Full} - {cursor.individualDeletedMessages} - {dispatcher.redeliveryMessages} - {the positions in inflight Replay Reading}, right?

Should we add metrics to describe how much memory individuallySentPositions usage?

Should we add a mechanism to stop delivering messages to the client if individuallySentPositions uses more memory than expected?

@equanz Sorry for the late reply. Could you take a look at these questions?

individuallySentPositions = {Range-Full} - {cursor.individualDeletedMessages} - {dispatcher.redeliveryMessages} - {the positions in inflight Replay Reading}, right?

Roughly, the {cursor.individualDeletedMessages} is not subtracted. It is initialized by individualDeletedMessages. Messages scheduled to be sent are pushed to this field.
After, if the first range is contiguous to the last sent position, remove the first range and update the last sent position to the range's upper bound.
So, if it is not initialized by individualDeletedMessages, then the last sent position can be stuck because of "sent-hole".

More specifically, the details may differ because the definitions are different.

Should we add metrics to describe how much memory individuallySentPositions usage?

I'll add these metrics to the subscription stats.

/** The last sent position of the cursor. This is for Key_Shared subscription. */ public String lastSentPosition; /** Set of individually sent ranges. This is for Key_Shared subscription. */ public String individuallySentPositions;

Should we add a mechanism to stop delivering messages to the client if individuallySentPositions uses more memory than expected?

I referred to the definition of the individualDeletedMessages. It has no limitation to persist on the memory(not the storage).
(Of course, we can add the limitation if necessary.)

@equanz

I'll add these metrics to the subscription stats.

/** The last sent position of the cursor. This is for Key_Shared subscription. */
public String lastSentPosition;

/** Set of individually sent ranges. This is for Key_Shared subscription. */
public String individuallySentPositions;

But individuallySentPositions in topic stats doesn't accurately reflect how much memory individuallySentPositions uses, right?

If there are a huge number of elements in individuallySentPositions, it is possible that pulsar-admin topics stats might not work, because the response body is too large (we have encountered responses close to 200m).

@poorbarcode

But individuallySentPositions in topic stats doesn't accurately reflect how much memory individuallySentPositions uses, right?

Yes.
However, I think we can't do effective operations even if we can observe these metrics. What do you think?

If there are a huge number of elements in individuallySentPositions, it is possible that pulsar-admin topics stats might not work, because the response body is too large (we have encountered responses close to 200m).

Your concerns are correct. I'll reconsider any other approaches(e.g. add a new REST API to expose these stats, just output log as debug level).
BTW, it is not considered in stats(e.g. consumersAfterMarkDeletePosition, keyHashRanges) and stats-internal(e.g. individuallyDeletedMessages). We should care about these stats, too.

@equanz

it is not considered in stats(e.g. consumersAfterMarkDeletePosition, keyHashRanges) and stats-internal(e.g. individuallyDeletedMessages). We should care about these stats, too.

Yes, I'm planning to do it.

compress the individuallyDeletedMessages to save place using, see: [improve] [ml] compress individual ack info to make maintain more records #21105

A new PIP to limit the place usage of individuallyDeletedMessages, the PIP will be submitted today

Your concerns are correct. I'll reconsider any other approaches(e.g. add a new REST API to expose these stats, just output log as debug level).

After we added new metrics which indicate how much memory is used by individuallySentPositions. Push an alert when the memory limit is exceeded, so there are no scenarios that the HTTP API cannot handle

A new PIP to limit the place usage of individuallyDeletedMessages, the PIP will be submitted today

Okay.
If implementation is not far off, I will wait for your new design for reference.
(This PIP issue is one of bug fixes. So, I think we should fix it sooner if possible.)

Sure. It is #21118

@poorbarcode
Sorry for the late reply. I have reconsidered your comments.

This field is appended when messages sent are not contiguous, and is automatically removed when the delivery becomes contiguous.
The frequency of occurrence between instances where messages are not delivered and instances where messages are not acked, would likely differ.
We could add limitations as you mentioned. However, it raises questions about how often this would be effective.

Additionally, introducing such additional stoppage specifications for deliveries would impose more complex limitations on Key_Shared alone.
If these limitations work correctly, that would be ideal. However, past bugs suggest that these complexities often cause omission from consideration.
Hence, if these added limitations do not effectively contribute, it may be better to consider not imposing them in the first place.

github-actions · 2023-10-05T01:48:19Z

The pr had no activity for 30 days, mark with Stale label.

equanz · 2023-11-20T10:20:55Z

~~If the discussion does not seem to return to active, I will start the voting phase once.~~ => I'll wait for approval in this PR.
This PIP issue is one of bug fixes(not improvement). So, I think we should fix it sooner if possible.

poorbarcode · 2023-11-27T02:42:08Z

I'll wait for approval in this PR.
This PIP issue is one of bug fixes(not improvement). So, I think we should fix it sooner if possible.

Sure, agree

equanz · 2023-11-28T08:11:31Z

I have opened a voting thread, so please check it out.
https://lists.apache.org/thread/45x056t8njjnzflbkhkofh00gcy4z5g6

equanz · 2023-12-25T05:19:38Z

@nkurihar @poorbarcode @codelipenghui

The vote was closed by 3 binding +1. Could you merge this proposal?
https://lists.apache.org/thread/ovckl6gdfppffvq0wk2l2lp5tq9hz8gb

poorbarcode · 2023-12-25T09:07:03Z

@nkurihar @poorbarcode @codelipenghui
The vote was closed by 3 binding +1. Could you merge this proposal?
https://lists.apache.org/thread/ovckl6gdfppffvq0wk2l2lp5tq9hz8gb

Sure, merging

equanz · 2024-02-07T05:30:10Z

I opened implementation PR #21953.

docs: add pip-282.md

56087fe

github-actions bot added type/PIP doc-not-needed Your PR changes do not impact docs labels Jul 11, 2023

equanz mentioned this pull request Jul 11, 2023

[fix][broker] Fix recentlyJoinedConsumers to address the out-of-order issue #20179

Closed

15 tasks

docs: add ML discussion thread link

6469681

poorbarcode assigned equanz Jul 11, 2023

poorbarcode added this to the 3.1.0 milestone Jul 11, 2023

poorbarcode requested review from poorbarcode and codelipenghui July 11, 2023 06:13

poorbarcode reviewed Jul 17, 2023

View reviewed changes

pip/pip-282.md Outdated Show resolved Hide resolved

pip/pip-282.md Outdated Show resolved Hide resolved

pip/pip-282.md Show resolved Hide resolved

pip/pip-282.md Show resolved Hide resolved

docs: address comments

986dd76

Technoboy- modified the milestones: 3.1.0, 3.2.0 Jul 31, 2023

poorbarcode added the type/bug The PR fixed a bug or issue reported a bug label Aug 3, 2023

poorbarcode reviewed Aug 28, 2023

View reviewed changes

poorbarcode changed the title ~~[pip][design] PIP-282: Change definition of the recently joined consumers position~~ [improve][pip] PIP-282: Change definition of the recently joined consumers position Sep 4, 2023

github-actions bot added the Stale label Oct 5, 2023

docs: add ML voting thread link

a6fa8fb

poorbarcode mentioned this pull request Dec 5, 2023

[fix][broker] Key-shared subscription must follow consumer redelivery as per shared sub semantic #21657

Open

15 tasks

poorbarcode approved these changes Dec 19, 2023

View reviewed changes

Technoboy- modified the milestones: 3.2.0, 3.3.0 Dec 22, 2023

poorbarcode merged commit 4e96e7f into apache:master Dec 25, 2023
19 checks passed

equanz deleted the pip_change_definition_of_recently_joined_consumers_position branch December 25, 2023 09:34

equanz mentioned this pull request Jan 23, 2024

[fix][broker] Introduce the last sent position to fix message ordering issues in Key_Shared (PIP-282) #21953

Merged

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[improve][pip] PIP-282: Change definition of the recently joined consumers position #20776

[improve][pip] PIP-282: Change definition of the recently joined consumers position #20776

equanz commented Jul 11, 2023

equanz commented Jul 31, 2023

poorbarcode Aug 28, 2023

poorbarcode Aug 28, 2023

equanz Sep 1, 2023

poorbarcode Sep 1, 2023 •

edited

Loading

equanz Sep 1, 2023

poorbarcode Sep 1, 2023

equanz Sep 4, 2023

poorbarcode Sep 4, 2023

equanz Nov 14, 2023

github-actions bot commented Oct 5, 2023

equanz commented Nov 20, 2023 •

edited

Loading

poorbarcode commented Nov 27, 2023

equanz commented Nov 28, 2023

equanz commented Dec 25, 2023

poorbarcode commented Dec 25, 2023

equanz commented Feb 7, 2024 •

edited

Loading

[improve][pip] PIP-282: Change definition of the recently joined consumers position #20776

[improve][pip] PIP-282: Change definition of the recently joined consumers position #20776

Conversation

equanz commented Jul 11, 2023

Does this pull request potentially affect one of the following parts:

Documentation

equanz commented Jul 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poorbarcode Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 5, 2023

equanz commented Nov 20, 2023 • edited Loading

poorbarcode commented Nov 27, 2023

equanz commented Nov 28, 2023

equanz commented Dec 25, 2023

poorbarcode commented Dec 25, 2023

equanz commented Feb 7, 2024 • edited Loading

poorbarcode Sep 1, 2023 •

edited

Loading

equanz commented Nov 20, 2023 •

edited

Loading

equanz commented Feb 7, 2024 •

edited

Loading