[improve][transactions] Add command to list transaction coordinators #17522

nicoloboschi · 2022-09-07T15:22:14Z

Fixes #17513

Motivation

At the moment there's no way to list all the transaction coordinators and to understand to which broker own.

Modifications

New endpoint `transactions/coordinators´ to get all the coordinators
- Under the hood it does a lookup to the TC partitioned topic
New data object ´TransactionCoordinatorInfo´ -> only contains the brokerServiceUrl field
Added the equivalent client call in the pulsar-client admin
Added the equivalent client call in the pulsar-admin CLI coordinators-list

doc-not-needed

eolivelli · 2022-09-08T06:57:06Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TransactionsBase.java

+                .thenAccept(map -> {
+                    if (map.isEmpty()) {
+                        asyncResponse.resume(new RestException(Response.Status.NOT_FOUND,
+                                "Transaction coordinator not found"));


This is not actually a "problem" to be reported as 404.
we can simply return an empty result.

how can we get to this situation ? when the partitions are not loaded by any broker ?

true, removed

eolivelli · 2022-09-08T06:58:32Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/v3/AdminApiTransactionTest.java

+    public void testListTransactionCoordinators() throws Exception {
+        initTransaction(4);
+        final Map<Integer, TransactionCoordinatorInfo> result = admin
+                .transactions().listTransactionCoordinatorsAsync().get();


should we use Awaitility here ?
are we guaranteed that when we reach this point all the partitions are loaded ?

in the initTransaction method we wait for all the topic partitions to be loaded

Awaitility.await().until(() -> pulsar.getTransactionMetadataStoreService().getStores().size() == coordinatorSize);

eolivelli · 2022-09-08T06:59:22Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TransactionsBase.java

+                        return;
+                    }
+                    Map<Integer, TransactionCoordinatorInfo> result = new HashMap<>();
+                    map.forEach((topicPartition, brokerServiceUrl) -> {


I think that we should return something also for the partitions that are not currently "loaded"

eolivelli · 2022-09-08T07:00:36Z

pulsar-client-admin-api/src/main/java/org/apache/pulsar/client/admin/Transactions.java

+     *
+     * @return the transaction coordinators list.
+     */
+    Map<Integer, TransactionCoordinatorInfo> listTransactionCoordinators() throws PulsarAdminException;


question (not a request for a change):
why returning a Map and not a List ?
I don't think that returning a Map has a particular advantage over returning a List

moved to List

eolivelli · 2022-09-08T07:00:53Z

...min-api/src/main/java/org/apache/pulsar/common/policies/data/TransactionCoordinatorInfo.java

+@NoArgsConstructor
+@AllArgsConstructor
+public class TransactionCoordinatorInfo {
+    private String brokerServiceUrl;


I would add the id as well, it would make it easier further processing downstream

congbobo184 · 2022-09-13T02:12:16Z

@nicoloboschi hi, why not add the broker address into org.apache.pulsar.common.policies.data.TransactionCoordinatorStats, in this way, we don't need to add the new interface.

github-actions · 2022-10-13T02:18:35Z

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

nicoloboschi · 2022-10-21T13:45:06Z

@nicoloboschi hi, why not add the broker address into org.apache.pulsar.common.policies.data.TransactionCoordinatorStats, in this way, we don't need to add the new interface.

The stats output is not needed here and they're hard to read considering you have multiple coordinators. Adding the related broker to the stat object requires a lookup request. I'd prefer to not add it

@eolivelli PTAL again

eolivelli

LGTM

nicoloboschi · 2022-10-24T13:52:16Z

/pulsarbot rerun-failure-checks

codecov-commenter · 2022-10-24T18:39:05Z

Codecov Report

Merging #17522 (d4f415d) into master (b31c5a6) will increase coverage by 0.08%.
The diff coverage is 70.83%.

@@             Coverage Diff              @@
##             master   #17522      +/-   ##
============================================
+ Coverage     46.98%   47.07%   +0.08%     
- Complexity    10343    10382      +39     
============================================
  Files           692      692              
  Lines         67766    67790      +24     
  Branches       7259     7258       -1     
============================================
+ Hits          31842    31913      +71     
+ Misses        32344    32315      -29     
+ Partials       3580     3562      -18

Flag	Coverage Δ
unittests	`47.07% <70.83%> (+0.08%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...che/pulsar/broker/admin/impl/TransactionsBase.java	`71.97% <66.66%> (-0.42%)`	⬇️
...rg/apache/pulsar/broker/admin/v3/Transactions.java	`77.23% <100.00%> (+0.56%)`	⬆️
...lsar/broker/service/StickyKeyConsumerSelector.java	`50.00% <0.00%> (-50.00%)`	⬇️
...e/HashRangeExclusiveStickyKeyConsumerSelector.java	`0.00% <0.00%> (-36.93%)`	⬇️
...oker/loadbalance/LoadResourceQuotaUpdaterTask.java	`44.44% <0.00%> (-33.34%)`	⬇️
...lsar/broker/loadbalance/impl/ThresholdShedder.java	`3.26% <0.00%> (-30.07%)`	⬇️
...rg/apache/pulsar/broker/lookup/v1/TopicLookup.java	`60.00% <0.00%> (-13.34%)`	⬇️
...e/pulsar/broker/service/EntryBatchIndexesAcks.java	`82.14% <0.00%> (-10.72%)`	⬇️
...org/apache/pulsar/broker/loadbalance/LoadData.java	`58.33% <0.00%> (-8.34%)`	⬇️
...he/pulsar/client/impl/PartitionedProducerImpl.java	`30.34% <0.00%> (-5.13%)`	⬇️
... and 46 more

congbobo184 · 2022-10-25T08:29:38Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/v3/AdminApiTransactionTest.java

+        initTransaction(4);
+        final List<TransactionCoordinatorInfo> result = admin
+                .transactions().listTransactionCoordinatorsAsync().get();
+        System.out.println("result" + result);


congbobo184 · 2022-10-25T08:55:39Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TransactionsBase.java

+                    return getPulsarResources()
+                            .getTopicResources()
+                            .getExistingPartitions(SystemTopicNames.TRANSACTION_COORDINATOR_ASSIGN);
+                })
+                .thenAccept(allPartitions -> {
+                    allPartitions
+                            .stream()
+                            .filter(partition -> SystemTopicNames
+                                    .isTransactionCoordinatorAssign(TopicName.get(partition)))
+                            .forEach(partition -> {
+                                final int coordinatorId = TopicName.getPartitionIndex(partition);
+                                if (!result.containsKey(coordinatorId)) {
+                                    result.put(coordinatorId,
+                                            new TransactionCoordinatorInfo(coordinatorId, null));
+                                }
+                            });
+                    asyncResponse.resume(result.values());


why do we need to check again?

Because there may be partitions not currently loaded by any broker but it's valuable for the user to know that exists

sorry for late response, lookupPartitionedTopicAsync has already return all the partition, it will not missing. are you test it will miss?

you're right. I left only the lookup PTAL

nicoloboschi · 2022-10-25T15:57:18Z

@congbobo184 I fixed the debug line and answered to your question

…pache#17522) * [improve][transactions] Add command to list transaction coordinators url * Address pr's comment and rebase * checkstyle * remove debug * Remove useless call and fix style * style (cherry picked from commit 1db89d7)

nicoloboschi force-pushed the cli-transaction-coordinators branch from 6a05d5d to ede029b Compare September 7, 2022 15:23

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 7, 2022

eolivelli requested a review from congbobo184 September 8, 2022 06:54

eolivelli requested changes Sep 8, 2022

View reviewed changes

nicoloboschi force-pushed the cli-transaction-coordinators branch from ede029b to 7b1659d Compare September 8, 2022 20:35

congbobo184 assigned nicoloboschi Sep 13, 2022

congbobo184 added area/transaction doc-required Your PR changes impact docs and you will update later. area/admin and removed doc-not-needed Your PR changes do not impact docs labels Sep 13, 2022

congbobo184 added this to the 2.12.0 milestone Sep 13, 2022

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 13, 2022

github-actions bot added the Stale label Oct 13, 2022

nicoloboschi force-pushed the cli-transaction-coordinators branch from 7b1659d to f3f9c78 Compare October 21, 2022 11:04

nicoloboschi removed the Stale label Oct 21, 2022

eolivelli approved these changes Oct 21, 2022

View reviewed changes

nicoloboschi added ready-to-test and removed doc-required Your PR changes impact docs and you will update later. labels Oct 24, 2022

nicoloboschi closed this Oct 24, 2022

nicoloboschi reopened this Oct 24, 2022

congbobo184 reviewed Oct 25, 2022

View reviewed changes

nicoloboschi added 3 commits November 10, 2022 09:54

[improve][transactions] Add command to list transaction coordinators url

0b66883

Address pr's comment and rebase

edef5ed

checkstyle

df78855

nicoloboschi added 2 commits November 10, 2022 09:54

remove debug

2407cb0

Remove useless call and fix style

de0300a

nicoloboschi force-pushed the cli-transaction-coordinators branch from 8adec32 to de0300a Compare November 10, 2022 15:47

style

d4f415d

congbobo184 approved these changes Nov 11, 2022

View reviewed changes

nicoloboschi merged commit 1db89d7 into apache:master Nov 11, 2022

nicoloboschi deleted the cli-transaction-coordinators branch December 1, 2022 16:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[improve][transactions] Add command to list transaction coordinators #17522

[improve][transactions] Add command to list transaction coordinators #17522

nicoloboschi commented Sep 7, 2022

eolivelli Sep 8, 2022

nicoloboschi Oct 21, 2022

eolivelli Sep 8, 2022

nicoloboschi Oct 21, 2022

eolivelli Sep 8, 2022

nicoloboschi Oct 21, 2022

eolivelli Sep 8, 2022

nicoloboschi Oct 21, 2022

eolivelli Sep 8, 2022

nicoloboschi Oct 21, 2022

congbobo184 commented Sep 13, 2022

github-actions bot commented Oct 13, 2022

nicoloboschi commented Oct 21, 2022

eolivelli left a comment

nicoloboschi commented Oct 24, 2022

codecov-commenter commented Oct 24, 2022 •

edited

congbobo184 Oct 25, 2022

congbobo184 Oct 25, 2022

nicoloboschi Oct 25, 2022

congbobo184 Oct 26, 2022

nicoloboschi Nov 10, 2022

nicoloboschi commented Oct 25, 2022

[improve][transactions] Add command to list transaction coordinators #17522

[improve][transactions] Add command to list transaction coordinators #17522

Conversation

nicoloboschi commented Sep 7, 2022

Motivation

Modifications

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

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

congbobo184 commented Sep 13, 2022

github-actions bot commented Oct 13, 2022

nicoloboschi commented Oct 21, 2022

eolivelli left a comment

Choose a reason for hiding this comment

nicoloboschi commented Oct 24, 2022

codecov-commenter commented Oct 24, 2022 • edited

Codecov Report

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

nicoloboschi commented Oct 25, 2022

codecov-commenter commented Oct 24, 2022 •

edited