You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/05/20 06:53:03 UTC
[GitHub] [pulsar] congbobo184 opened a new pull request #10650: [Transactionn] Transaction admin api component in topic status
congbobo184 opened a new pull request #10650:
URL: https://github.com/apache/pulsar/pull/10650
## Motivation
Transaction add admin api `getCoordinatorStatus`
## implement
```
## Motivation
Transaction add admin api `getComponentInTopicStatus`
## implement
```
/** The transaction coordinatorId. */
public long coordinatorId;
/** The state of this transaction metadataStore. */
public String state;
/** The sequenceId of transaction metadataStore. */
public long sequenceId;
/** The low water mark of transaction metadataStore. */
public long lowWaterMark;
```
This is transaction buffer metrics.
### Verifying this change
Add the tests for it
Does this pull request potentially affect one of the following parts:
If yes was chosen, please highlight the changes
Dependencies (does it add or upgrade a dependency): (no)
The public API: (no)
The schema: (no)
The default values of configurations: (no)
The wire protocol: (no)
The rest endpoints: (no)
The admin cli options: (yes)
Anything that affects deployment: (no)
```
This is transaction buffer metrics.
### Verifying this change
Add the tests for it
Does this pull request potentially affect one of the following parts:
If yes was chosen, please highlight the changes
Dependencies (does it add or upgrade a dependency): (no)
The public API: (no)
The schema: (no)
The default values of configurations: (no)
The wire protocol: (no)
The rest endpoints: (no)
The admin cli options: (yes)
Anything that affects deployment: (no)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #10650: [Transactionn] Transaction admin api component in topic status
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #10650:
URL: https://github.com/apache/pulsar/pull/10650#discussion_r639309938
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTransactions.java
##########
@@ -84,6 +109,8 @@ void run() throws Exception {
public CmdTransactions(Supplier<PulsarAdmin> admin) {
super("transactions", admin);
jcommander.addCommand("coordinator-status", new GetCoordinatorStatus());
+ jcommander.addCommand("transaction-buffer-status", new GetTransactionBufferStatus());
Review comment:
```suggestion
jcommander.addCommand("transaction-buffer-stats", new GetTransactionBufferStats());
```
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/TransactionPendingAckStatus.java
##########
@@ -0,0 +1,25 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.common.policies.data;
+
+public class TransactionPendingAckStatus {
Review comment:
```suggestion
public class TransactionPendingAckStats {
```
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/TransactionBufferStatus.java
##########
@@ -0,0 +1,31 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.common.policies.data;
+
+public class TransactionBufferStatus {
Review comment:
```suggestion
public class TransactionBufferStats {
```
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTransactions.java
##########
@@ -84,6 +109,8 @@ void run() throws Exception {
public CmdTransactions(Supplier<PulsarAdmin> admin) {
super("transactions", admin);
jcommander.addCommand("coordinator-status", new GetCoordinatorStatus());
+ jcommander.addCommand("transaction-buffer-status", new GetTransactionBufferStatus());
+ jcommander.addCommand("pending-ack-status", new GetPendingAckStatus());
Review comment:
```suggestion
jcommander.addCommand("pending-ack-stats", new GetPendingAckStats());
```
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTransactions.java
##########
@@ -42,6 +42,31 @@ void run() throws Exception {
}
}
+ @Parameters(commandDescription = "Get transaction buffer status")
+ private class GetTransactionBufferStatus extends CliCommand {
Review comment:
```suggestion
private class GetTransactionBufferStats extends CliCommand {
```
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTransactions.java
##########
@@ -42,6 +42,31 @@ void run() throws Exception {
}
}
+ @Parameters(commandDescription = "Get transaction buffer status")
+ private class GetTransactionBufferStatus extends CliCommand {
+ @Parameter(names = {"-t", "--topic"}, description = "the topic", required = true)
+ private String topic;
+
+ @Override
+ void run() throws Exception {
+ print(getAdmin().transactions().getTransactionBufferStatus(topic));
+ }
+ }
+
+ @Parameters(commandDescription = "Get transaction pending ack status")
+ private class GetPendingAckStatus extends CliCommand {
Review comment:
```suggestion
private class GetPendingAckStats extends CliCommand {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 commented on a change in pull request #10650: [Transactionn] Transaction admin api component in topic status
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on a change in pull request #10650:
URL: https://github.com/apache/pulsar/pull/10650#discussion_r638840777
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v3/Transactions.java
##########
@@ -55,6 +55,55 @@ public void getCoordinatorStatus(@Suspended final AsyncResponse asyncResponse,
internalGetCoordinatorStatus(asyncResponse, authoritative, coordinatorId);
}
+ @GET
+ @Path("/transactionInBufferStats")
+ @ApiOperation(value = "Get transaction state in transaction buffer.")
+ @ApiResponses(value = {@ApiResponse(code = 403, message = "Don't have admin permission"),
+ @ApiResponse(code = 404, message = "Tenant or cluster or namespace or topic doesn't exist"),
+ @ApiResponse(code = 503, message = "This Broker is not configured "
+ + "with transactionCoordinatorEnabled=true."),
+ @ApiResponse(code = 307, message = "Topic don't owner by this broker!"),
+ @ApiResponse(code = 501, message = "Topic is not a persistent topic!"),
+ @ApiResponse(code = 409, message = "Concurrent modification")})
+ public void getTransactionInBufferStats(@Suspended final AsyncResponse asyncResponse,
+ @QueryParam("authoritative")
+ @DefaultValue("false") boolean authoritative,
+ @QueryParam("mostSigBits")
Review comment:
client don't know the txnId is consists of coordinatorId and sequenceId. it only know a TxnID consists of mostSigBits and leastSigBits. It's better to understand.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #10650: [Transactionn] Transaction admin api component in topic status
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #10650:
URL: https://github.com/apache/pulsar/pull/10650#discussion_r638715132
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v3/Transactions.java
##########
@@ -55,6 +55,55 @@ public void getCoordinatorStatus(@Suspended final AsyncResponse asyncResponse,
internalGetCoordinatorStatus(asyncResponse, authoritative, coordinatorId);
}
+ @GET
+ @Path("/transactionInBufferStats")
+ @ApiOperation(value = "Get transaction state in transaction buffer.")
+ @ApiResponses(value = {@ApiResponse(code = 403, message = "Don't have admin permission"),
+ @ApiResponse(code = 404, message = "Tenant or cluster or namespace or topic doesn't exist"),
+ @ApiResponse(code = 503, message = "This Broker is not configured "
+ + "with transactionCoordinatorEnabled=true."),
+ @ApiResponse(code = 307, message = "Topic don't owner by this broker!"),
+ @ApiResponse(code = 501, message = "Topic is not a persistent topic!"),
+ @ApiResponse(code = 409, message = "Concurrent modification")})
+ public void getTransactionInBufferStats(@Suspended final AsyncResponse asyncResponse,
+ @QueryParam("authoritative")
+ @DefaultValue("false") boolean authoritative,
+ @QueryParam("mostSigBits")
Review comment:
isn't it a little bit awkward to to pass separately mostSigBits and leastSigBits in a REST API ?
can we using a string representation ?
do we have other examples in the REST API about splitting the TxID into the two parts ?
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v3/Transactions.java
##########
@@ -55,6 +55,55 @@ public void getCoordinatorStatus(@Suspended final AsyncResponse asyncResponse,
internalGetCoordinatorStatus(asyncResponse, authoritative, coordinatorId);
}
+ @GET
+ @Path("/transactionInBufferStats")
+ @ApiOperation(value = "Get transaction state in transaction buffer.")
+ @ApiResponses(value = {@ApiResponse(code = 403, message = "Don't have admin permission"),
+ @ApiResponse(code = 404, message = "Tenant or cluster or namespace or topic doesn't exist"),
+ @ApiResponse(code = 503, message = "This Broker is not configured "
+ + "with transactionCoordinatorEnabled=true."),
+ @ApiResponse(code = 307, message = "Topic don't owner by this broker!"),
+ @ApiResponse(code = 501, message = "Topic is not a persistent topic!"),
Review comment:
"Topic is not a persistent topic"
501 is kind of "internal server error", probably we should return something like "Not allowed" or "Bad request"
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v3/Transactions.java
##########
@@ -55,6 +55,55 @@ public void getCoordinatorStatus(@Suspended final AsyncResponse asyncResponse,
internalGetCoordinatorStatus(asyncResponse, authoritative, coordinatorId);
}
+ @GET
+ @Path("/transactionInBufferStats")
+ @ApiOperation(value = "Get transaction state in transaction buffer.")
+ @ApiResponses(value = {@ApiResponse(code = 403, message = "Don't have admin permission"),
+ @ApiResponse(code = 404, message = "Tenant or cluster or namespace or topic doesn't exist"),
+ @ApiResponse(code = 503, message = "This Broker is not configured "
+ + "with transactionCoordinatorEnabled=true."),
+ @ApiResponse(code = 307, message = "Topic don't owner by this broker!"),
Review comment:
what about
"Topic is not owned by this broker"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #10650: [Transactionn] Transaction admin api component in topic status
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #10650:
URL: https://github.com/apache/pulsar/pull/10650#issuecomment-848553437
@congbobo184 Could you please update the title of the PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 edited a comment on pull request #10650: [Transactionn] Transaction admin api component in topic status
Posted by GitBox <gi...@apache.org>.
congbobo184 edited a comment on pull request #10650:
URL: https://github.com/apache/pulsar/pull/10650#issuecomment-848412059
@Anonymitaet please review doc thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on a change in pull request #10650: [Transactionn] Transaction admin api component in topic status
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on a change in pull request #10650:
URL: https://github.com/apache/pulsar/pull/10650#discussion_r639370981
##########
File path: pulsar-client-admin-api/src/main/java/org/apache/pulsar/client/admin/Transactions.java
##########
@@ -71,4 +73,21 @@
*/
CompletableFuture<TransactionMetadata> getTransactionMetadata(TxnID txnID);
+ /**
+ * Get transaction buffer stats.
+ *
+ * @param topic the topic of getting transaction buffer stats
+ * @return the future stats of transaction buffer in topic.
+ */
+ CompletableFuture<TransactionBufferStats> getTransactionBufferStats(String topic);
+
+ /**
+ * Get transaction pending ack stats.
+ *
+ * @param topic the topic of this transaction pending ack stats
+ * @param subName the topic of this transaction pending ack stats
Review comment:
```suggestion
* @param subName the subName of this transaction pending ack stats
```
is `subName` accurate? or `subscription name`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli merged pull request #10650: [Transactionn] Transaction admin api getTransactionBufferStatus and getPendingAckStatus
Posted by GitBox <gi...@apache.org>.
eolivelli merged pull request #10650:
URL: https://github.com/apache/pulsar/pull/10650
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #10650: [Transactionn] Transaction admin api getTransactionBufferStatus and getPendingAckStatus
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #10650:
URL: https://github.com/apache/pulsar/pull/10650#discussion_r639493159
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v3/Transactions.java
##########
@@ -55,6 +55,55 @@ public void getCoordinatorStatus(@Suspended final AsyncResponse asyncResponse,
internalGetCoordinatorStatus(asyncResponse, authoritative, coordinatorId);
}
+ @GET
+ @Path("/transactionInBufferStats")
+ @ApiOperation(value = "Get transaction state in transaction buffer.")
+ @ApiResponses(value = {@ApiResponse(code = 403, message = "Don't have admin permission"),
+ @ApiResponse(code = 404, message = "Tenant or cluster or namespace or topic doesn't exist"),
+ @ApiResponse(code = 503, message = "This Broker is not configured "
+ + "with transactionCoordinatorEnabled=true."),
+ @ApiResponse(code = 307, message = "Topic don't owner by this broker!"),
+ @ApiResponse(code = 501, message = "Topic is not a persistent topic!"),
+ @ApiResponse(code = 409, message = "Concurrent modification")})
+ public void getTransactionInBufferStats(@Suspended final AsyncResponse asyncResponse,
+ @QueryParam("authoritative")
+ @DefaultValue("false") boolean authoritative,
+ @QueryParam("mostSigBits")
Review comment:
@eolivelli Currently all the txn Id in the admin API expressed as `mostSigBits` and `leastSigBits`. The client also can get the `mostSigBits` and `leastSigBits` of a transactionId when using the transactions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 commented on pull request #10650: [Transactionn] Transaction admin api component in topic status
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on pull request #10650:
URL: https://github.com/apache/pulsar/pull/10650#issuecomment-848412059
@Anonymitaet please review doc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org