[improve][broker] Optimize high CPU usage when consuming from topics with ongoing txn

[coderzc]

Motivation

We found the CPU of the broker busy with calling ManagedLedgerImpl.internalReadFromLedger and broker checked the readPosition > maxPosition and then triggered the readMoreEntries call again leading to the broker looping to call readMoreEntries. But in ManagedCursorImpl.asyncReadEntriesWithSkipOrWait we have checked no more data to read via hasMoreEntries(). I think this case may be caused by maxReadPosition < lastConfirmedPosition when the topic exists ongoing Txn. So I think maxPosition <= readPosition we should not read entries immediately, instead, we delay calling read entries.

Modifications

If maxPosition < readPosition then delayed trigger readEntries.

Test Code:

    @Test
    public void testSlowTxn() throws Exception {
        String topic = NAMESPACE1 + "/testSlowTxn";
        @Cleanup
        ProducerImpl<byte[]> producer = (ProducerImpl<byte[]>) pulsarClient.newProducer()
                .topic(topic)
                .sendTimeout(1, TimeUnit.SECONDS)
                .create();

        @Cleanup
        Consumer<byte[]> consumer = pulsarClient.newConsumer()
                .topic(topic)
                .subscriptionName("test")
                .subscriptionType(SubscriptionType.Shared)
                .subscribe();

        Transaction transaction = pulsarClient.newTransaction().withTransactionTimeout(10, TimeUnit.MINUTES)
                .build().get();

        producer.newMessage(transaction).value("Hello Pulsar!".getBytes()).send();

        Thread.sleep(10 * 60 * 1000);

        transaction.commit().get();
        producer.close();
        admin.topics().delete(topic, true);
    }

CPU usage before applying this change:

flamegraph: https://drive.google.com/file/d/1nNb4MOdbZB7mO4fWitts2UpjzutdT22O/view?usp=sharing

CPU usage after applying this change:

flamegraph: https://drive.google.com/file/d/1AndMJuSMXhOImf3T0hg_E7YeCyslTaNI/view?usp=sharing

Verifying this change

• Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

• Added integration tests for end-to-end deployment with large payloads (10MB)
• Extended integration test for recovery after broker failure

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

If the box was checked, please highlight the changes

• Dependencies (add or upgrade a dependency)
• The public API
• The schema
• The default values of configurations
• The threading model
• The binary protocol
• The REST endpoints
• The admin CLI options
• The metrics
• Anything that affects deployment

Documentation

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

Matching PR in forked repository

PR in forked repository:

[thetumbled]

Same problem as #22944?

[coderzc]

Same problem as #22944?

Looks like yes, I will review #22944

[lhotari]

LGTM Good catch @coderzc!

[coderzc]

This is a quick fix, it is only effective for managedLedgerNewEntriesCheckDelayInMillis > 0 we can merge pr first and we can continue to review #22944