Pin order executor in LedgerHandle

[AnonHxy]

Descriptions of the changes in this PR:

Motivation

Pin the order executor in LedgerHandle to save the choose thread overhead. For details reason:

We could call executeOrdered() frequently, e.g., when in asyncReadEntries. And we would compute the thread index from ledgerId every time when call executeOrdered(). So pin the order executor will save the thread index compution time.

Changes

• Choose the order executor in LedgerHandle constructor method and save it as a final field
• Override getMainWorkerPool() in MockBookKeeper to fix UT NPE exception when call this.executor = clientCtx.getMainWorkerPool().chooseThread(ledgerId);

[AnonHxy]

rerun failure checks

[AnonHxy]

PTAL @StevenLuMT @eolivelli

[hangc0276]

@AnonHxy Would you please provide more context about the reason to pin the executor? thanks.

[AnonHxy]

@AnonHxy Would you please provide more context about the reason to pin the executor? thanks.

Sure. It was inspired by this PR apache/pulsar#18078. and I have provided more detail reasons in the descriptions of this PR @hangc0276

[AnonHxy]

PTAL @hangc0276 @eolivelli , thanks

[StevenLuMT]

when the execution mode changes, the performance change significantly ? ledgerID will block reading. Is it expected?

[AnonHxy]

when the execution mode changes, the performance change significantly ? ledgerID will block reading. Is it expected?

I think this patch will not change the executeion mode. It just make the execute thread as a final field, which is a obvious improvement. But I am not sure how much can be improved. @StevenLuMT

[hangc0276]

Overall LGTM. This change saves the cost of choosing threads in executeOrdered, and I wonder if choosing threads costs a lot of CPU resources and whether this change can improve the performance or not.

[AnonHxy]

Overall LGTM. This change saves the cost of choosing threads in executeOrdered, and I wonder if choosing threads costs a lot of CPU resources and whether this change can improve the performance or not.

I tested it using one bookie client to add entry async like following. It looks that it was not a significant improvement from the result.

 for (int i = 0; i < Integer.MAX_VALUE; ++i) {
                handle.asyncAddEntry("Some entry data".getBytes(), new AsyncCallback.AddCallback() {
                    @Override
                    public void addComplete(int rc, LedgerHandle lh, long entryId, Object ctx) {

                    }
                }, null);
 }

[hangc0276]

Overall LGTM. This change saves the cost of choosing threads in executeOrdered, and I wonder if choosing threads costs a lot of CPU resources and whether this change can improve the performance or not.

I tested it using one bookie client to add entry async like following. It looks that it was not a significant improvement from the result.

 for (int i = 0; i < Integer.MAX_VALUE; ++i) {
                handle.asyncAddEntry("Some entry data".getBytes(), new AsyncCallback.AddCallback() {
                    @Override
                    public void addComplete(int rc, LedgerHandle lh, long entryId, Object ctx) {

                    }
                }, null);
 }

@AnonHxy If there is no significant performance improvement, I think we can hold on this change.

[hangc0276]

@AnonHxy Do you have any updates?

[AnonHxy]

I think we can hold on this change.

OK. But I think this change is a positive optimization anyway, while the proform result is not obvious ... @hangc0276

[hangc0276]

LGTM

[StevenLuMT]

Good catch,LGTM

[AnonHxy]

@eolivelli @dlg99 @merlimat PTAL, thanks:)

[eolivelli]

LGTM