[GitHub] [pulsar-client-cpp] BewareMyPower commented on a diff in pull request #129: [improve] Add configuration to limit times of client's lookup redirection.
BewareMyPower commented on code in PR #129:
URL: https://github.com/apache/pulsar-client-cpp/pull/129#discussion_r1080910162
##
lib/HandlerBase.h:
##
@@ -113,13 +113,14 @@ class HandlerBase {
enum State
{
-NotStarted,
-Pending,
-Ready,
-Closing,
-Closed,
-Failed,
-Producer_Fenced
+NotStarted, // Not initialized, in Java client:
HandlerState.State.Uninitialized
+Pending, // Client connecting to broker, in Java client:
HandlerState.State.Connecting
+Ready,// Handler is being used, in Java client:
HandlerState.State.Ready
+Closing, // Close cmd has been sent to broker, in Java
client: HandlerState.State.Closing
+Closed, // Broker acked the close, in Java client:
HandlerState.State.Closed
+Failed, // Handler is failed, in Java client:
HandlerState.State.Failed
+Producer_Fenced, // The producer has been fenced by the broker
Review Comment:
Why not use a name `ProducerFenced`? It's better to keep the variable naming
style consistent.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] eolivelli opened a new pull request, #19284: [bugfix] AbstractBatchedMetadataStore - use AlreadyClosedException instead of IllegalStateException
eolivelli opened a new pull request, #19284: URL: https://github.com/apache/pulsar/pull/19284 ### Motivation When the broker is shutting down AbstractBatchedMetadataStore completes exceptionally the pending operations with a generic IllegalStateException. All the code that is depending on the MetadataStore usually expects instances of MetadataStoreException and they may not properly react to this error. This is generally not a big deal because the broker is shutting down, but we should improve it ### Modifications Complete the pending operations with AlreadyClosedException instead of IllegalStateException ### Verifying this change This change is a trivial rework / code cleanup without any test coverage. ### 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` - [x] `doc-not-needed` - [ ] `doc-complete` -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar-client-cpp] BewareMyPower commented on a diff in pull request #129: [improve] Add configuration to limit times of client's lookup redirection.
BewareMyPower commented on code in PR #129:
URL: https://github.com/apache/pulsar-client-cpp/pull/129#discussion_r1080906832
##
include/pulsar/ClientConfiguration.h:
##
@@ -64,25 +64,25 @@ class PULSAR_PUBLIC ClientConfiguration {
*
* @param timeout the timeout after which the operation will be considered
as failed
*/
-ClientConfiguration& setOperationTimeoutSeconds(int timeout);
+ClientConfiguration& setOperationTimeoutSeconds(int32_t timeout);
Review Comment:
I think we should open another PR to change the types of these existing
methods. Thought it won't be a breaking change to replace `int` with `int32_t`.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar-client-cpp] BewareMyPower commented on pull request #170: [feature] Support set ServiceUrlProvider when create client.
BewareMyPower commented on PR #170: URL: https://github.com/apache/pulsar-client-cpp/pull/170#issuecomment-1396560323 In short, a natural design is to save the `ServiceUrlProvider` in `Client` and call `getServiceUrl()` method (in C++ it's just a function invocation) each time we need a service URL. Maybe we need to maintain a key-value (`serviceUrl -> ServiceNameResolver`), when the `serviceUrlProvider()` returns a different result from the key, creating a new `ServiceNameResolver` for related lookup requests. -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar-client-cpp] BewareMyPower commented on pull request #170: [feature] Support set ServiceUrlProvider when create client.
BewareMyPower commented on PR #170: URL: https://github.com/apache/pulsar-client-cpp/pull/170#issuecomment-1396556883 I have sent an email: https://lists.apache.org/thread/cojz6qxpc7v0j8vy0w476ttt0f89zr3z -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar-client-cpp] shibd commented on pull request #170: [feature] Support set ServiceUrlProvider when create client.
shibd commented on PR #170: URL: https://github.com/apache/pulsar-client-cpp/pull/170#issuecomment-1396555980 If you feel redundant to expose this method, I can remove it. What do you think? /cc @BewareMyPower -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar-client-cpp] BewareMyPower merged pull request #139: [feat] Support Dead Letter Topic.
BewareMyPower merged PR #139: URL: https://github.com/apache/pulsar-client-cpp/pull/139 -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[pulsar-client-cpp] branch main updated (3e5c9c7 -> 9ed6a45)
This is an automated email from the ASF dual-hosted git repository.
xyz pushed a change to branch main
in repository https://gitbox.apache.org/repos/asf/pulsar-client-cpp.git
from 3e5c9c7 [feat] Reader support readeNextAsync interface. (#176)
add 9ed6a45 [feat] Support Dead Letter Topic. (#139)
No new revisions were added by this update.
Summary of changes:
include/pulsar/ConsumerConfiguration.h | 39 ++
...{ConsumerEventListener.h => DeadLetterPolicy.h} | 52 ++-
include/pulsar/DeadLetterPolicyBuilder.h | 94 +
include/pulsar/ProducerConfiguration.h | 7 +-
lib/Commands.cc| 6 +-
lib/Commands.h | 3 +-
lib/ConsumerConfiguration.cc | 6 +
lib/ConsumerConfigurationImpl.h| 1 +
lib/ConsumerImpl.cc| 174 -
lib/ConsumerImpl.h | 14 +
lib/DeadLetterPolicyBuilder.cc | 55 +++
lib/{MessagesImpl.h => DeadLetterPolicyImpl.cc}| 33 +-
...hReceivePolicyImpl.h => DeadLetterPolicyImpl.h} | 11 +-
lib/MessageIdImpl.h| 25 ++
lib/MultiTopicsConsumerImpl.cc | 18 +-
lib/NegativeAcksTracker.cc | 2 +-
lib/ProducerConfigurationImpl.h| 1 +
lib/ProducerImpl.cc| 10 +-
tests/ConsumerConfigurationTest.cc | 16 +
tests/DeadLetterPolicyTest.cc | 45 +++
tests/DeadLetterQueueTest.cc | 392 +
tests/PulsarFriend.h | 6 +
22 files changed, 957 insertions(+), 53 deletions(-)
copy include/pulsar/{ConsumerEventListener.h => DeadLetterPolicy.h} (51%)
create mode 100644 include/pulsar/DeadLetterPolicyBuilder.h
create mode 100644 lib/DeadLetterPolicyBuilder.cc
copy lib/{MessagesImpl.h => DeadLetterPolicyImpl.cc} (59%)
copy lib/{BatchReceivePolicyImpl.h => DeadLetterPolicyImpl.h} (83%)
create mode 100644 tests/DeadLetterPolicyTest.cc
create mode 100644 tests/DeadLetterQueueTest.cc
[GitHub] [pulsar-client-cpp] shibd commented on pull request #170: [feature] Support set ServiceUrlProvider when create client.
shibd commented on PR #170:
URL:
https://github.com/apache/pulsar-client-cpp/pull/170#issuecomment-1396552263
> If we want to implement AutoClusterFailover and ControlledClusterFailover
in future, we actually don't need to implement a function that returns the
service URL. Instead, we only need to implement the following methods:
Yes, Actually, the `ServiceUrlProvider ` implemented here is just convenient
for the user to return a function in order to get the service URL from
elsewhere.
If we do not expose the set `ServiceUrlProvider`, It's no problem too.
Use `ServiceUrlProvider`
```c++
Client client([]() {
// get service URL from store.
auto serviceUrl = xxx
return serviceUrl;
})
```
`ServiceUrlProvider` is not used
```c++
// get service URL from store.
auto serviceUrl = xxx
Client client(serviceUrl);
```
I exposed it just for ease of use.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] eolivelli commented on issue #16249: Pulsar Admin API client should support retries on operations
eolivelli commented on issue #16249: URL: https://github.com/apache/pulsar/issues/16249#issuecomment-1396546157 I think that it makes sense to retry some (most) of the admin operations. We do retries while producing messages why can't we do them in the admin API ? -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #19280: [improve][broker] AuthenticationState authenticate data once
michaeljmarshall commented on PR #19280: URL: https://github.com/apache/pulsar/pull/19280#issuecomment-1396538310 This work will end up as multiple PRs and I think I've found a way to avoid confusing breaking changes. The first PR that was initially covered in this PR is here: https://github.com/apache/pulsar/pull/19282. The second is here https://github.com/apache/pulsar/pull/19283. I expect at least one more. -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] Hongten commented on issue #12628: Could not complete the operation. Number of retries has been exhausted. Failed reason: Maximum redirect reached: 5
Hongten commented on issue #12628: URL: https://github.com/apache/pulsar/issues/12628#issuecomment-1396532277 I encountered the same issue. I resolved the issue by renaming the `source` name. ``` docker run -it -p 6650:6650 -p 8080:8080 --mount source=pulsardata1,target=/pulsar/data --mount source=pulsarconf,target=/pulsar/conf apachepulsar/pulsar-all:2.10.1 bin/pulsar standalone ``` -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] michaeljmarshall opened a new pull request, #19283: [improve][broker] Documentation for AuthenticationState contract
michaeljmarshall opened a new pull request, #19283: URL: https://github.com/apache/pulsar/pull/19283 PIP 97: https://github.com/apache/pulsar/issues/12105 ### Motivation While working on PIP 97, I noticed that we do not have a clear contract for the `AuthenticationState` interface. We use the interface in one way in the `ServerCnx` class, but some of our official implementations do not align with the usage and the API. The PR itself provides my proposed authoritative documentation. When we agree on the content of this PR, I will follow up with some changes to the `TokenAuthenticationState` class and potentially others. ### Modifications * Add documentation on how the `AuthenticationState` interface will be used by Pulsar. ### Verifying this change I have studied the code closely to understand how it works, and more importantly, how it should work. ### Does this pull request potentially affect one of the following parts: This change could affect how plugins interact with the `AuthenticationState` object. ### Documentation - [x] `doc` This is a documentation change. -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #18130: [fix][broker] Fix update authentication data
michaeljmarshall commented on code in PR #18130:
URL: https://github.com/apache/pulsar/pull/18130#discussion_r1080873280
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##
@@ -707,73 +709,96 @@ private void completeConnect(int clientProtoVersion,
String clientVersion, boole
}
// According to auth result, send newConnected or newAuthChallenge command.
-private State doAuthentication(AuthData clientData,
- int clientProtocolVersion,
- String clientVersion) throws Exception {
-
+private CompletableFuture doAuthenticationAsync(AuthData clientData,
int clientProtocolVersion,
+ String
clientVersion) {
// The original auth state can only be set on subsequent auth attempts
(and only
// in presence of a proxy and if the proxy is forwarding the
credentials).
// In this case, the re-validation needs to be done against the
original client
-// credentials.
-boolean useOriginalAuthState = (originalAuthState != null);
-AuthenticationState authState = useOriginalAuthState ?
originalAuthState : this.authState;
-String authRole = useOriginalAuthState ? originalPrincipal :
this.authRole;
-AuthData brokerData = authState.authenticate(clientData);
-
-if (log.isDebugEnabled()) {
-log.debug("Authenticate using original auth state : {}, role =
{}", useOriginalAuthState, authRole);
-}
+// credentials, but we only can new an authentication state, because
some authentication data(TLS, SASL)
+// based on outside service.
+// If we can get the role from the authentication sate, the global
variable need to be updated.
-if (authState.isComplete()) {
-// Authentication has completed. It was either:
-// 1. the 1st time the authentication process was done, in which
case we'll send
-//a `CommandConnected` response
-// 2. an authentication refresh, in which case we need to refresh
authenticationData
-
-String newAuthRole = authState.getAuthRole();
-
-// Refresh the auth data.
-this.authenticationData = authState.getAuthDataSource();
+boolean useOriginalAuthState = (originalAuthState != null);
+if (state == State.Connected) {
+// For auth challenge, the authentication state requires to be
updated.
if (log.isDebugEnabled()) {
-log.debug("[{}] Auth data refreshed for role={}",
remoteAddress, this.authRole);
+log.debug("Refreshing authenticate state, original auth state:
{}, original auth role: {}, "
++ "auth role: {}",
+useOriginalAuthState, originalPrincipal, authRole);
}
-
-if (!useOriginalAuthState) {
-this.authRole = newAuthRole;
+try {
+if (useOriginalAuthState) {
+originalAuthState =
+
originalAuthenticationProvider.newAuthState(clientData, remoteAddress,
sslSession);
+} else {
+authState =
authenticationProvider.newAuthState(clientData, remoteAddress, sslSession);
Review Comment:
I created https://github.com/apache/pulsar/pull/19282 to start a concrete
discussion.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] michaeljmarshall opened a new pull request, #19282: [fix][broker] Let TokenAuthState update authenticationDataSource
michaeljmarshall opened a new pull request, #19282: URL: https://github.com/apache/pulsar/pull/19282 ### Motivation We use the result of `TokenAuthenticationState#getAuthDataSource` to pass to the `AuthorizationProvider`. For custom implementations, it is important that this information is up to date. The `TokenAuthenticationState` field `AuthenticationDataSource` is set on initialization and never again. Given that tokens can be refreshed, we should update the field `TokenAuthenticationState#authenticationDataSource` when the `authenticate` method is called. ### Modifications * Update the `authenticationDataSource` when `authenticate` is called. ### Verifying this change A new test is added to cover this change. ### Does this pull request potentially affect one of the following parts: This change will only affect third party `AuthorizationProvider` implementations. It's possible that it could break their integration, though unlikely. Note that we update the `authRole` when `authenticate` is called. ### Documentation - [x] `doc-not-needed` We do not document this kind of internal behavior anywhere. ### Matching PR in forked repository PR in forked repository: https://github.com/michaeljmarshall/pulsar/pull/15 -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar-client-cpp] BewareMyPower merged pull request #176: [feat] Reader support readeNextAsync interface.
BewareMyPower merged PR #176: URL: https://github.com/apache/pulsar-client-cpp/pull/176 -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[pulsar-client-cpp] branch main updated: [feat] Reader support readeNextAsync interface. (#176)
This is an automated email from the ASF dual-hosted git repository.
xyz pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/pulsar-client-cpp.git
The following commit(s) were added to refs/heads/main by this push:
new 3e5c9c7 [feat] Reader support readeNextAsync interface. (#176)
3e5c9c7 is described below
commit 3e5c9c71e5fd8662eed4430eaa249a2bf7936c66
Author: Baodi Shi
AuthorDate: Thu Jan 19 14:34:40 2023 +0800
[feat] Reader support readeNextAsync interface. (#176)
---
include/pulsar/Reader.h | 8 +++
include/pulsar/ReaderConfiguration.h | 1 +
lib/ConsumerImpl.cc | 2 +-
lib/ConsumerImpl.h | 2 +-
lib/ConsumerImplBase.h | 2 +-
lib/MultiTopicsConsumerImpl.cc | 2 +-
lib/MultiTopicsConsumerImpl.h| 2 +-
lib/Reader.cc| 8 +++
lib/ReaderImpl.cc| 8 +++
lib/ReaderImpl.h | 1 +
tests/ReaderTest.cc | 45
11 files changed, 76 insertions(+), 5 deletions(-)
diff --git a/include/pulsar/Reader.h b/include/pulsar/Reader.h
index 233da4f..4c124c9 100644
--- a/include/pulsar/Reader.h
+++ b/include/pulsar/Reader.h
@@ -29,6 +29,7 @@ class PulsarFriend;
class ReaderImpl;
typedef std::function
HasMessageAvailableCallback;
+typedef std::function
ReadNextCallback;
/**
* A Reader can be used to scan through all the messages currently available
in a topic.
@@ -68,6 +69,13 @@ class PULSAR_PUBLIC Reader {
*/
Result readNext(Message& msg, int timeoutMs);
+/**
+ * Read asynchronously the next message in the topic.
+ *
+ * @param callback
+ */
+void readNextAsync(ReadNextCallback callback);
+
/**
* Close the reader and stop the broker to push more messages
*
diff --git a/include/pulsar/ReaderConfiguration.h
b/include/pulsar/ReaderConfiguration.h
index 9ae8e1c..4f6464f 100644
--- a/include/pulsar/ReaderConfiguration.h
+++ b/include/pulsar/ReaderConfiguration.h
@@ -162,6 +162,7 @@ class PULSAR_PUBLIC ReaderConfiguration {
* Set the internal subscription name.
*
* @param internal subscriptionName
+ * Default value is reader-{random string}.
*/
void setInternalSubscriptionName(std::string internalSubscriptionName);
diff --git a/lib/ConsumerImpl.cc b/lib/ConsumerImpl.cc
index ccd082b..e7f6cf4 100644
--- a/lib/ConsumerImpl.cc
+++ b/lib/ConsumerImpl.cc
@@ -841,7 +841,7 @@ Result ConsumerImpl::receive(Message& msg) {
return res;
}
-void ConsumerImpl::receiveAsync(ReceiveCallback& callback) {
+void ConsumerImpl::receiveAsync(ReceiveCallback callback) {
Message msg;
// fail the callback if consumer is closing or closed
diff --git a/lib/ConsumerImpl.h b/lib/ConsumerImpl.h
index 4832f4e..6e1b8ff 100644
--- a/lib/ConsumerImpl.h
+++ b/lib/ConsumerImpl.h
@@ -94,7 +94,7 @@ class ConsumerImpl : public ConsumerImplBase {
const std::string& getTopic() const override;
Result receive(Message& msg) override;
Result receive(Message& msg, int timeout) override;
-void receiveAsync(ReceiveCallback& callback) override;
+void receiveAsync(ReceiveCallback callback) override;
void unsubscribeAsync(ResultCallback callback) override;
void acknowledgeAsync(const MessageId& msgId, ResultCallback callback)
override;
void acknowledgeAsync(const MessageIdList& messageIdList, ResultCallback
callback) override;
diff --git a/lib/ConsumerImplBase.h b/lib/ConsumerImplBase.h
index 5bc7e1b..9cf63a3 100644
--- a/lib/ConsumerImplBase.h
+++ b/lib/ConsumerImplBase.h
@@ -51,7 +51,7 @@ class ConsumerImplBase : public HandlerBase, public
std::enable_shared_from_this
virtual const std::string& getSubscriptionName() const = 0;
virtual Result receive(Message& msg) = 0;
virtual Result receive(Message& msg, int timeout) = 0;
-virtual void receiveAsync(ReceiveCallback& callback) = 0;
+virtual void receiveAsync(ReceiveCallback callback) = 0;
void batchReceiveAsync(BatchReceiveCallback callback);
virtual void unsubscribeAsync(ResultCallback callback) = 0;
virtual void acknowledgeAsync(const MessageId& msgId, ResultCallback
callback) = 0;
diff --git a/lib/MultiTopicsConsumerImpl.cc b/lib/MultiTopicsConsumerImpl.cc
index a013566..c7a656c 100644
--- a/lib/MultiTopicsConsumerImpl.cc
+++ b/lib/MultiTopicsConsumerImpl.cc
@@ -583,7 +583,7 @@ Result MultiTopicsConsumerImpl::receive(Message& msg, int
timeout) {
}
}
-void MultiTopicsConsumerImpl::receiveAsync(ReceiveCallback& callback) {
+void MultiTopicsConsumerImpl::receiveAsync(ReceiveCallback callback) {
Message msg;
// fail the callback if consumer is closing or closed
diff --git a/lib/MultiTopicsConsumerImpl.h b/lib/MultiTopicsConsumerImpl.h
index da42b74..50cdecf 100644
--- a/lib/MultiTopicsConsumerImpl.h
+++ b/lib/MultiTopicsConsumerImpl.h
@@ -65,7 +65,7 @@ class
[GitHub] [pulsar] sekikn commented on pull request #19281: [improve][io] Upgrade Alluxio to 2.8.1
sekikn commented on PR #19281: URL: https://github.com/apache/pulsar/pull/19281#issuecomment-1396502041 Originally I tried to upgrade Alluxio to 2.9.0, which is the latest version at this writing, but [Alluxio's metrics refers to the `sun.management` package since that version](https://github.com/Alluxio/alluxio/blob/v2.9.0/core/common/src/main/java/alluxio/metrics/MetricsSystem.java#L165) and I came across the following error during using it. ``` 2023-01-19T09:47:18,396+0900 [public/default/alluxio-sink-0] ERROR org.apache.pulsar.functions.instance.JavaInstanceRunnable - [public/default/alluxio-sink:0] Uncaught exception in Java Instance java.lang.IllegalAccessError: class alluxio.metrics.MetricsSystem (in unnamed module @0x8c05339) cannot access class sun.management.ManagementFactoryHelper (in module java.management) because module java.management does not export sun.management to unnamed module @0x8c05339 at alluxio.metrics.MetricsSystem.getDirectBufferPool(MetricsSystem.java:165) ~[?:?] at alluxio.metrics.MetricsSystem.(MetricsSystem.java:157) ~[?:?] at alluxio.client.file.FileSystemContext.initContext(FileSystemContext.java:316) ~[?:?] at alluxio.client.file.FileSystemContext.init(FileSystemContext.java:305) ~[?:?] at alluxio.client.file.FileSystemContext.create(FileSystemContext.java:256) ~[?:?] at alluxio.client.file.FileSystemContext.create(FileSystemContext.java:225) ~[?:?] at alluxio.client.file.FileSystemContext.create(FileSystemContext.java:207) ~[?:?] at alluxio.client.file.FileSystem$Factory.create(FileSystem.java:138) ~[?:?] at org.apache.pulsar.io.alluxio.sink.AlluxioSink.open(AlluxioSink.java:101) ~[?:?] at org.apache.pulsar.functions.instance.JavaInstanceRunnable.setupOutput(JavaInstanceRunnable.java:857) ~[org.apache.pulsar-pulsar-functions-instance-2.11.0.jar:2.11.0] at org.apache.pulsar.functions.instance.JavaInstanceRunnable.setup(JavaInstanceRunnable.java:224) ~[org.apache.pulsar-pulsar-functions-instance-2.11.0.jar:2.11.0] at org.apache.pulsar.functions.instance.JavaInstanceRunnable.run(JavaInstanceRunnable.java:259) ~[org.apache.pulsar-pulsar-functions-instance-2.11.0.jar:2.11.0] at java.lang.Thread.run(Thread.java:833) ~[?:?] ``` Probably I have to add the `--add-opens java.management/sun.management=ALL-UNNAMED` runtime option somewhere, but I'm not sure where it is yet. So let me upgrade it to 2.9.0+ in another 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar-client-cpp] BewareMyPower commented on pull request #170: [feature] Support set ServiceUrlProvider when create client.
BewareMyPower commented on PR #170: URL: https://github.com/apache/pulsar-client-cpp/pull/170#issuecomment-1396500856 > The initialize and close methods are redundant. I reviewed the `ServiceUrlProvider` in Java client again. **I have thought `getServiceUrl()` is called each time the service URL is needed (e.g. topic lookup) but it's actually not.** The update of the service URL is actually done by `PulsarClient#updateServiceUrl`, which is triggered by the periodical task of `ServiceUrlProvide#initialize`. If we want to implement `AutoClusterFailover` and `ControlledClusterFailover` in future, we actually don't need to implement a function that returns the service URL. Instead, we only need to implement the following methods: - PulsarClient#updateServiceUrl - PulsarClientImpl#updateAuthentication - PulsarClientImpl#updateTlsTrustCertsFilePath The way of using `ServiceUrlProvider` is bad. It's more like a provider that can modify the internal state of `PulsarClient`. For C++ client, this PR implements the similar way with Java that the `ServiceUrlProvider` (a function that returns the service URL) is only triggerred when constructing the `Client` instance. There will be no difference between ```c++ ServiceUrlProvider provider = /* ... */; Client client(provider()); ``` and ```c++ ServiceUrlProvider provider = /* ... */; String serviceUrl = provider(); Client client(serviceUrl); ``` l'm going to send an email to dev mail list to discuss. -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[pulsar] branch master updated (3c38ed5a4f6 -> a9b65196258)
This is an automated email from the ASF dual-hosted git repository. mmarshall pushed a change to branch master in repository https://gitbox.apache.org/repos/asf/pulsar.git from 3c38ed5a4f6 [feat][broker] Update AuthenticationProvider to simplify HTTP Authn (#19197) add a9b65196258 [cleanup][proxy] Remove unused AuthenticationDataSource variable (#19278) No new revisions were added by this update. Summary of changes: .../src/main/java/org/apache/pulsar/proxy/server/ProxyConnection.java | 3 --- 1 file changed, 3 deletions(-)
[GitHub] [pulsar] michaeljmarshall merged pull request #19278: [cleanup][proxy] Remove unused AuthenticationDataSource variable
michaeljmarshall merged PR #19278: URL: https://github.com/apache/pulsar/pull/19278 -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] sekikn opened a new pull request, #19281: [improve][io] Upgrade Alluxio to 2.8.1
sekikn opened a new pull request, #19281: URL: https://github.com/apache/pulsar/pull/19281 ### Motivation Currently, the Alluxio sink depends on Alluxio 2.7.3, which was released around a year ago. ### Modifications This PR upgrades Alluxio to 2.8.1 so that we can ensure the sink works with more recent version of Alluxio. ### Verifying this change - [x] Make sure that the change passes the CI checks. This change is already covered by existing tests, such as AlluxioSinkConfigTest and AlluxioSinkTest. ### Does this pull request potentially affect one of the following parts: *If the box was checked, please highlight the changes* - [x] Dependencies (add or upgrade a dependency) - Upgrade Alluxio 2.7.3 to 2.8.1. Its API changed a bit, so the PR addressed it too. - [ ] 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` - [x] `doc-not-needed` - [ ] `doc-complete` ### Matching PR in forked repository PR in forked repository: https://github.com/sekikn/incubator-pulsar/pull/10 The CI above failed on the BROKER_CLIENT_IMPL and BROKER_GROUP_1 tests, but they seem to be unrelated with this change. -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #18130: [fix][broker] Fix update authentication data
michaeljmarshall commented on code in PR #18130:
URL: https://github.com/apache/pulsar/pull/18130#discussion_r1080840353
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##
@@ -707,73 +709,96 @@ private void completeConnect(int clientProtoVersion,
String clientVersion, boole
}
// According to auth result, send newConnected or newAuthChallenge command.
-private State doAuthentication(AuthData clientData,
- int clientProtocolVersion,
- String clientVersion) throws Exception {
-
+private CompletableFuture doAuthenticationAsync(AuthData clientData,
int clientProtocolVersion,
+ String
clientVersion) {
// The original auth state can only be set on subsequent auth attempts
(and only
// in presence of a proxy and if the proxy is forwarding the
credentials).
// In this case, the re-validation needs to be done against the
original client
-// credentials.
-boolean useOriginalAuthState = (originalAuthState != null);
-AuthenticationState authState = useOriginalAuthState ?
originalAuthState : this.authState;
-String authRole = useOriginalAuthState ? originalPrincipal :
this.authRole;
-AuthData brokerData = authState.authenticate(clientData);
-
-if (log.isDebugEnabled()) {
-log.debug("Authenticate using original auth state : {}, role =
{}", useOriginalAuthState, authRole);
-}
+// credentials, but we only can new an authentication state, because
some authentication data(TLS, SASL)
+// based on outside service.
+// If we can get the role from the authentication sate, the global
variable need to be updated.
-if (authState.isComplete()) {
-// Authentication has completed. It was either:
-// 1. the 1st time the authentication process was done, in which
case we'll send
-//a `CommandConnected` response
-// 2. an authentication refresh, in which case we need to refresh
authenticationData
-
-String newAuthRole = authState.getAuthRole();
-
-// Refresh the auth data.
-this.authenticationData = authState.getAuthDataSource();
Review Comment:
> I thought the same thing, but the Proxy's role usually is a superuser, if
we don't check the client's role, this is overstepping its authority, and the
client can get the lookup data and topic data.
>
> If we don't consider this case, I agree with you.
Can you describe the attack vector? From my perspective, I don't see how a
client would be able to assume the proxy's role.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #18130: [fix][broker] Fix update authentication data
michaeljmarshall commented on code in PR #18130:
URL: https://github.com/apache/pulsar/pull/18130#discussion_r1080839635
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##
@@ -707,73 +709,96 @@ private void completeConnect(int clientProtoVersion,
String clientVersion, boole
}
// According to auth result, send newConnected or newAuthChallenge command.
-private State doAuthentication(AuthData clientData,
- int clientProtocolVersion,
- String clientVersion) throws Exception {
-
+private CompletableFuture doAuthenticationAsync(AuthData clientData,
int clientProtocolVersion,
+ String
clientVersion) {
// The original auth state can only be set on subsequent auth attempts
(and only
// in presence of a proxy and if the proxy is forwarding the
credentials).
// In this case, the re-validation needs to be done against the
original client
-// credentials.
-boolean useOriginalAuthState = (originalAuthState != null);
-AuthenticationState authState = useOriginalAuthState ?
originalAuthState : this.authState;
-String authRole = useOriginalAuthState ? originalPrincipal :
this.authRole;
-AuthData brokerData = authState.authenticate(clientData);
-
-if (log.isDebugEnabled()) {
-log.debug("Authenticate using original auth state : {}, role =
{}", useOriginalAuthState, authRole);
-}
+// credentials, but we only can new an authentication state, because
some authentication data(TLS, SASL)
+// based on outside service.
+// If we can get the role from the authentication sate, the global
variable need to be updated.
-if (authState.isComplete()) {
-// Authentication has completed. It was either:
-// 1. the 1st time the authentication process was done, in which
case we'll send
-//a `CommandConnected` response
-// 2. an authentication refresh, in which case we need to refresh
authenticationData
-
-String newAuthRole = authState.getAuthRole();
-
-// Refresh the auth data.
-this.authenticationData = authState.getAuthDataSource();
+boolean useOriginalAuthState = (originalAuthState != null);
+if (state == State.Connected) {
+// For auth challenge, the authentication state requires to be
updated.
if (log.isDebugEnabled()) {
-log.debug("[{}] Auth data refreshed for role={}",
remoteAddress, this.authRole);
+log.debug("Refreshing authenticate state, original auth state:
{}, original auth role: {}, "
++ "auth role: {}",
+useOriginalAuthState, originalPrincipal, authRole);
}
-
-if (!useOriginalAuthState) {
-this.authRole = newAuthRole;
+try {
+if (useOriginalAuthState) {
+originalAuthState =
+
originalAuthenticationProvider.newAuthState(clientData, remoteAddress,
sslSession);
+} else {
+authState =
authenticationProvider.newAuthState(clientData, remoteAddress, sslSession);
Review Comment:
I actually don't think third-party plugins are going to be a problem here.
The `AuthenticationState` object has lived the whole life of the `ServerCnx`
already, so updating the `TokenAuthenticationState#authenticate` implementation
shouldn't be a problem. Or, when you say "third-party plugins", are you
referencing to those plugins relying on the `TokenAuthenticationState`? Even
so, I think it's a pretty easy change, and I'd even consider it a bug since the
`AuthenticationDataSource` is static even though we try to update it in the
`ServerCnx`.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #18130: [fix][broker] Fix update authentication data
michaeljmarshall commented on code in PR #18130:
URL: https://github.com/apache/pulsar/pull/18130#discussion_r1080838475
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##
@@ -707,73 +709,96 @@ private void completeConnect(int clientProtoVersion,
String clientVersion, boole
}
// According to auth result, send newConnected or newAuthChallenge command.
-private State doAuthentication(AuthData clientData,
- int clientProtocolVersion,
- String clientVersion) throws Exception {
-
+private CompletableFuture doAuthenticationAsync(AuthData clientData,
int clientProtocolVersion,
+ String
clientVersion) {
// The original auth state can only be set on subsequent auth attempts
(and only
// in presence of a proxy and if the proxy is forwarding the
credentials).
// In this case, the re-validation needs to be done against the
original client
-// credentials.
-boolean useOriginalAuthState = (originalAuthState != null);
-AuthenticationState authState = useOriginalAuthState ?
originalAuthState : this.authState;
-String authRole = useOriginalAuthState ? originalPrincipal :
this.authRole;
-AuthData brokerData = authState.authenticate(clientData);
-
-if (log.isDebugEnabled()) {
-log.debug("Authenticate using original auth state : {}, role =
{}", useOriginalAuthState, authRole);
-}
+// credentials, but we only can new an authentication state, because
some authentication data(TLS, SASL)
+// based on outside service.
Review Comment:
> For the current design, the `originalAuthData` is credible by the proxy
forwarding authentication data. This authentication data was completed in the
proxy and then sent this authentication data to the broker, so we don't check
the authentication data in the broker.
My primary objection is that the setting required to enter this code block
is called `isAuthenticateOriginalAuthData`. Note that we are already verifying
most auth data in the case that the `newAuthState` method actually
authenticates the data. That leaves out cases like SASL which do not
authentication on `AuthenticationState` initialization because it is a
multi-stage auth. However, I've just realized that forwarding the
authentication information will especially not work with SASL because the proxy
only forwards the _last_ `AuthData` that it receives:
https://github.com/apache/pulsar/blob/d6fcdb8e1e192e0d9e2fbe6307d273b27ed2930f/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConnection.java#L426-L440
This seems like a gap in the protocol where multi-staged authentication is
not able to be properly processed through the proxy. However, if its a gap,
that means it is something that could be improved without breaking anything.
One question is whether it would help to have a distinct stages where we
first authenticate the proxy and then authenticate its original client (when
configured to do so). That could help to open up the design and to make the
protocol a bit clearer.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #17831: [fix][proxy] Fix refresh client auth
michaeljmarshall commented on code in PR #17831:
URL: https://github.com/apache/pulsar/pull/17831#discussion_r1080836860
##
pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConnection.java:
##
@@ -523,18 +529,63 @@ remoteAddress, protocolVersionToAdvertise,
getRemoteEndpointProtocolVersion(),
@Override
protected void handleAuthResponse(CommandAuthResponse authResponse) {
-checkArgument(state == State.Connecting);
checkArgument(authResponse.hasResponse());
checkArgument(authResponse.getResponse().hasAuthData() &&
authResponse.getResponse().hasAuthMethodName());
if (LOG.isDebugEnabled()) {
LOG.debug("Received AuthResponse from {}, auth method: {}",
-remoteAddress, authResponse.getResponse().getAuthMethodName());
+remoteAddress,
authResponse.getResponse().getAuthMethodName());
}
try {
AuthData clientData =
AuthData.of(authResponse.getResponse().getAuthData());
doAuthentication(clientData);
+if (service.getConfiguration().isForwardAuthorizationCredentials()
Review Comment:
An interesting consequence of this change is that the `ProxyConnection` goes
back into the `Connecting` state if the authentication takes multiple steps.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #17831: [fix][proxy] Fix refresh client auth
michaeljmarshall commented on code in PR #17831:
URL: https://github.com/apache/pulsar/pull/17831#discussion_r1080836860
##
pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConnection.java:
##
@@ -523,18 +529,63 @@ remoteAddress, protocolVersionToAdvertise,
getRemoteEndpointProtocolVersion(),
@Override
protected void handleAuthResponse(CommandAuthResponse authResponse) {
-checkArgument(state == State.Connecting);
checkArgument(authResponse.hasResponse());
checkArgument(authResponse.getResponse().hasAuthData() &&
authResponse.getResponse().hasAuthMethodName());
if (LOG.isDebugEnabled()) {
LOG.debug("Received AuthResponse from {}, auth method: {}",
-remoteAddress, authResponse.getResponse().getAuthMethodName());
+remoteAddress,
authResponse.getResponse().getAuthMethodName());
}
try {
AuthData clientData =
AuthData.of(authResponse.getResponse().getAuthData());
doAuthentication(clientData);
+if (service.getConfiguration().isForwardAuthorizationCredentials()
Review Comment:
An interesting consequence of this change is that the `ProxyConnection` goes
back into the `Connecting` state.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #18130: [fix][broker] Fix update authentication data
michaeljmarshall commented on code in PR #18130:
URL: https://github.com/apache/pulsar/pull/18130#discussion_r1073882511
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##
@@ -707,73 +709,96 @@ private void completeConnect(int clientProtoVersion,
String clientVersion, boole
}
// According to auth result, send newConnected or newAuthChallenge command.
-private State doAuthentication(AuthData clientData,
- int clientProtocolVersion,
- String clientVersion) throws Exception {
-
+private CompletableFuture doAuthenticationAsync(AuthData clientData,
int clientProtocolVersion,
+ String
clientVersion) {
// The original auth state can only be set on subsequent auth attempts
(and only
// in presence of a proxy and if the proxy is forwarding the
credentials).
// In this case, the re-validation needs to be done against the
original client
-// credentials.
-boolean useOriginalAuthState = (originalAuthState != null);
-AuthenticationState authState = useOriginalAuthState ?
originalAuthState : this.authState;
-String authRole = useOriginalAuthState ? originalPrincipal :
this.authRole;
-AuthData brokerData = authState.authenticate(clientData);
-
-if (log.isDebugEnabled()) {
-log.debug("Authenticate using original auth state : {}, role =
{}", useOriginalAuthState, authRole);
-}
+// credentials, but we only can new an authentication state, because
some authentication data(TLS, SASL)
+// based on outside service.
Review Comment:
> For `originalAuthentication`, we don't call the authentication checks.
Isn't this a problem though? We aren't really authenticating the
`originalAuthData` if we don't call the `authenticate` method and make sure
authentication is "complete". In the `ProxyConnectionToBroker` case, we can
send back `AuthChallenge` In the event that the proxy is forwarding
authentication information, we can issue `AuthChallenge` responses. It might
not work so easily in the `ProxyLookupRequests` state.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar-client-cpp] BewareMyPower commented on a diff in pull request #170: [feature] Support set ServiceUrlProvider when create client.
BewareMyPower commented on code in PR #170:
URL: https://github.com/apache/pulsar-client-cpp/pull/170#discussion_r1080827497
##
include/pulsar/Client.h:
##
@@ -66,6 +67,31 @@ class PULSAR_PUBLIC Client {
*/
Client(const std::string& serviceUrl, const ClientConfiguration&
clientConfiguration);
+/**
+ * Create a Pulsar client object connecting to the specified cluster
address and using the default
+ * configuration.
+ *
+ * Instead of specifying a static service URL string (with {@link
#serviceUrl(String)}), an application
+ * can pass a {@link ServiceUrlProvider} function that dynamically provide
a service URL.
+ *
+ * @param serviceUrlProvider The serviceUrlProvider used to generate
ServiceUrl.
+ * @throw std::invalid_argument if `serviceUrlProvider()` return a invalid
url.
+ */
+Client(ServiceUrlProvider serviceUrlProvider);
Review Comment:
```suggestion
/**
* @see Client(ServiceUrlProvider, const ClientConfiguration&)
*/
Client(ServiceUrlProvider serviceUrlProvider) :
Client(serviceUrlProvider, ClientConfiguration{}) {}
```
Avoid unnecessary duplicated code and API docs.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[pulsar] branch master updated (ea4f7eba335 -> 3c38ed5a4f6)
This is an automated email from the ASF dual-hosted git repository. mmarshall pushed a change to branch master in repository https://gitbox.apache.org/repos/asf/pulsar.git from ea4f7eba335 [fix][io] Fix the Alluxio sink to write messages successfully after the first file rotation (#19247) add 3c38ed5a4f6 [feat][broker] Update AuthenticationProvider to simplify HTTP Authn (#19197) No new revisions were added by this update. Summary of changes: .../authentication/AuthenticationProvider.java | 37 ++-- .../AuthenticationProviderToken.java | 17 .../authentication/AuthenticationService.java | 80 ++-- .../pulsar/broker/web/AuthenticationFilter.java| 48 +- .../AuthenticationProviderTokenTest.java | 8 +- .../broker/auth/AuthenticationServiceTest.java | 104 + 6 files changed, 226 insertions(+), 68 deletions(-)
[GitHub] [pulsar] michaeljmarshall merged pull request #19197: [feat][broker] Update AuthenticationProvider to simplify HTTP Authn
michaeljmarshall merged PR #19197: URL: https://github.com/apache/pulsar/pull/19197 -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar-client-cpp] BewareMyPower commented on a diff in pull request #170: [feature] Support set ServiceUrlProvider when create client.
BewareMyPower commented on code in PR #170:
URL: https://github.com/apache/pulsar-client-cpp/pull/170#discussion_r1080826113
##
include/pulsar/Client.h:
##
@@ -66,6 +67,31 @@ class PULSAR_PUBLIC Client {
*/
Client(const std::string& serviceUrl, const ClientConfiguration&
clientConfiguration);
+/**
+ * Create a Pulsar client object connecting to the specified cluster
address and using the default
+ * configuration.
+ *
+ * Instead of specifying a static service URL string (with {@link
#serviceUrl(String)}), an application
+ * can pass a {@link ServiceUrlProvider} function that dynamically provide
a service URL.
+ *
+ * @param serviceUrlProvider The serviceUrlProvider used to generate
ServiceUrl.
+ * @throw std::invalid_argument if `serviceUrlProvider()` return a invalid
url.
+ */
+Client(ServiceUrlProvider serviceUrlProvider);
Review Comment:
LGTM. Before this PR, `serviceUrl` is a required argument. But after this
PR, only one of `serviceUrl` and `serviceUrlProvider` should be provided.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #19197: [feat][broker] Update AuthenticationProvider to simplify HTTP Authn
michaeljmarshall commented on code in PR #19197:
URL: https://github.com/apache/pulsar/pull/19197#discussion_r1080825354
##
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java:
##
@@ -160,6 +163,20 @@ public String authenticate(AuthenticationDataSource
authData) throws Authenticat
}
}
+@Override
+public boolean authenticateHttpRequest(HttpServletRequest request,
HttpServletResponse response) throws Exception {
Review Comment:
Thank you for sharing your concerns. It is great to have this context
written down in the PR for future reference.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] nodece commented on pull request #19278: [cleanup][proxy] Remove unused AuthenticationDataSource variable
nodece commented on PR #19278: URL: https://github.com/apache/pulsar/pull/19278#issuecomment-1396442310 /pulsarbot rerun-failure-checks -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar-client-cpp] shibd commented on a diff in pull request #170: [feature] Support set ServiceUrlProvider when create client.
shibd commented on code in PR #170:
URL: https://github.com/apache/pulsar-client-cpp/pull/170#discussion_r1080780926
##
lib/ClientImpl.cc:
##
@@ -497,6 +497,20 @@ void ClientImpl::getPartitionsForTopicAsync(const
std::string& topic, GetPartiti
std::placeholders::_2, topicName, callback));
}
+Result ClientImpl::updateServiceUrl(const std::string& serviceUrl) {
+LOG_INFO("Updating service URL to " << serviceUrl);
+
+try {
+serviceNameResolver_.updateServiceUrl(serviceUrl);
+lookupServicePtr_->updateServiceUrl(serviceUrl);
Review Comment:
Ok, I will remove `updateServiceUrl` method of `LookupService `.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] nodece commented on a diff in pull request #19197: [feat][broker] Update AuthenticationProvider to simplify HTTP Authn
nodece commented on code in PR #19197:
URL: https://github.com/apache/pulsar/pull/19197#discussion_r1080780426
##
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java:
##
@@ -160,6 +163,20 @@ public String authenticate(AuthenticationDataSource
authData) throws Authenticat
}
}
+@Override
+public boolean authenticateHttpRequest(HttpServletRequest request,
HttpServletResponse response) throws Exception {
Review Comment:
Thank you for your explanation. This looks good to me.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] nodece commented on a diff in pull request #18130: [fix][broker] Fix update authentication data
nodece commented on code in PR #18130:
URL: https://github.com/apache/pulsar/pull/18130#discussion_r1080775696
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##
@@ -707,73 +709,96 @@ private void completeConnect(int clientProtoVersion,
String clientVersion, boole
}
// According to auth result, send newConnected or newAuthChallenge command.
-private State doAuthentication(AuthData clientData,
- int clientProtocolVersion,
- String clientVersion) throws Exception {
-
+private CompletableFuture doAuthenticationAsync(AuthData clientData,
int clientProtocolVersion,
+ String
clientVersion) {
// The original auth state can only be set on subsequent auth attempts
(and only
// in presence of a proxy and if the proxy is forwarding the
credentials).
// In this case, the re-validation needs to be done against the
original client
-// credentials.
-boolean useOriginalAuthState = (originalAuthState != null);
-AuthenticationState authState = useOriginalAuthState ?
originalAuthState : this.authState;
-String authRole = useOriginalAuthState ? originalPrincipal :
this.authRole;
-AuthData brokerData = authState.authenticate(clientData);
-
-if (log.isDebugEnabled()) {
-log.debug("Authenticate using original auth state : {}, role =
{}", useOriginalAuthState, authRole);
-}
+// credentials, but we only can new an authentication state, because
some authentication data(TLS, SASL)
+// based on outside service.
+// If we can get the role from the authentication sate, the global
variable need to be updated.
-if (authState.isComplete()) {
-// Authentication has completed. It was either:
-// 1. the 1st time the authentication process was done, in which
case we'll send
-//a `CommandConnected` response
-// 2. an authentication refresh, in which case we need to refresh
authenticationData
-
-String newAuthRole = authState.getAuthRole();
-
-// Refresh the auth data.
-this.authenticationData = authState.getAuthDataSource();
+boolean useOriginalAuthState = (originalAuthState != null);
+if (state == State.Connected) {
+// For auth challenge, the authentication state requires to be
updated.
if (log.isDebugEnabled()) {
-log.debug("[{}] Auth data refreshed for role={}",
remoteAddress, this.authRole);
+log.debug("Refreshing authenticate state, original auth state:
{}, original auth role: {}, "
++ "auth role: {}",
+useOriginalAuthState, originalPrincipal, authRole);
}
-
-if (!useOriginalAuthState) {
-this.authRole = newAuthRole;
+try {
+if (useOriginalAuthState) {
+originalAuthState =
+
originalAuthenticationProvider.newAuthState(clientData, remoteAddress,
sslSession);
+} else {
+authState =
authenticationProvider.newAuthState(clientData, remoteAddress, sslSession);
Review Comment:
Right. We can fix built-in plugins easily, but we also need to consider
third-party plugins.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] nodece commented on a diff in pull request #18130: [fix][broker] Fix update authentication data
nodece commented on code in PR #18130:
URL: https://github.com/apache/pulsar/pull/18130#discussion_r1080774793
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##
@@ -707,73 +709,96 @@ private void completeConnect(int clientProtoVersion,
String clientVersion, boole
}
// According to auth result, send newConnected or newAuthChallenge command.
-private State doAuthentication(AuthData clientData,
- int clientProtocolVersion,
- String clientVersion) throws Exception {
-
+private CompletableFuture doAuthenticationAsync(AuthData clientData,
int clientProtocolVersion,
+ String
clientVersion) {
// The original auth state can only be set on subsequent auth attempts
(and only
// in presence of a proxy and if the proxy is forwarding the
credentials).
// In this case, the re-validation needs to be done against the
original client
-// credentials.
-boolean useOriginalAuthState = (originalAuthState != null);
-AuthenticationState authState = useOriginalAuthState ?
originalAuthState : this.authState;
-String authRole = useOriginalAuthState ? originalPrincipal :
this.authRole;
-AuthData brokerData = authState.authenticate(clientData);
-
-if (log.isDebugEnabled()) {
-log.debug("Authenticate using original auth state : {}, role =
{}", useOriginalAuthState, authRole);
-}
+// credentials, but we only can new an authentication state, because
some authentication data(TLS, SASL)
+// based on outside service.
+// If we can get the role from the authentication sate, the global
variable need to be updated.
-if (authState.isComplete()) {
-// Authentication has completed. It was either:
-// 1. the 1st time the authentication process was done, in which
case we'll send
-//a `CommandConnected` response
-// 2. an authentication refresh, in which case we need to refresh
authenticationData
-
-String newAuthRole = authState.getAuthRole();
-
-// Refresh the auth data.
-this.authenticationData = authState.getAuthDataSource();
Review Comment:
> However, I am not sure why that is necessary, and as we've discussed, this
is not even done correctly for the auth data source.
For the current design, the `authenticationDataSource` is credible by the
proxy forwarding authentication data. This authentication data was completed in
the proxy and then sent this authentication data to the broker, so we don't
check the authentication data in the broker.
> When the proxy is forwarding authentication data, it seems sufficient to
verify that the proxy's authentication data is valid for a proxy as a one time
check during connection initialization. All subsequent auth challenges will go
to the client anyway.
I thought the same thing, but the Proxy's role usually is a superuser, if we
don't check the client's role, this is overstepping its authority, and the
client can get the lookup data and topic data.
If we don't consider this case, I agree with you.
> No. We must check both the originalAuthState and authState, if you don't
check, you will overstep your authority.
I agree that we currently check authorization on the proxy role and the
original principal as well as with the proxy's authenticationDataSource and the
originalAuthenticationDataSource. However, I am not sure why that is necessary,
and as we've discussed, this is not even done correctly for the auth data
source.
> What is the case that a client will overstep its authority?
When the proxy is forwarding authentication data, it seems sufficient to
verify that the proxy's authentication data is valid for a proxy as a one time
check during connection initialization. All subsequent auth challenges will go
to the client anyway.
> If the proxy is unable to forward authentication data, then the broker
will only have the proxy's authentication data and the `originalPrincipal`.
Right.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] nodece commented on a diff in pull request #18130: [fix][broker] Fix update authentication data
nodece commented on code in PR #18130:
URL: https://github.com/apache/pulsar/pull/18130#discussion_r1080769754
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##
@@ -707,73 +709,96 @@ private void completeConnect(int clientProtoVersion,
String clientVersion, boole
}
// According to auth result, send newConnected or newAuthChallenge command.
-private State doAuthentication(AuthData clientData,
- int clientProtocolVersion,
- String clientVersion) throws Exception {
-
+private CompletableFuture doAuthenticationAsync(AuthData clientData,
int clientProtocolVersion,
+ String
clientVersion) {
// The original auth state can only be set on subsequent auth attempts
(and only
// in presence of a proxy and if the proxy is forwarding the
credentials).
// In this case, the re-validation needs to be done against the
original client
-// credentials.
-boolean useOriginalAuthState = (originalAuthState != null);
-AuthenticationState authState = useOriginalAuthState ?
originalAuthState : this.authState;
-String authRole = useOriginalAuthState ? originalPrincipal :
this.authRole;
-AuthData brokerData = authState.authenticate(clientData);
-
-if (log.isDebugEnabled()) {
-log.debug("Authenticate using original auth state : {}, role =
{}", useOriginalAuthState, authRole);
-}
+// credentials, but we only can new an authentication state, because
some authentication data(TLS, SASL)
+// based on outside service.
Review Comment:
For the current design, the `originalAuthData` is credible by the proxy
forwarding authentication data. This authentication data was completed in the
proxy and then sent this authentication data to the broker, so we don't check
the authentication data in the broker.
For `ProxyLookupRequests` state, see
https://github.com/apache/pulsar/pull/17831. I forward the `AuthChallenge`
command to the user's client by the ProxyClient and Proxy Server.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar-client-cpp] shibd commented on pull request #139: [feat] Support Dead Letter Topic.
shibd commented on PR #139: URL: https://github.com/apache/pulsar-client-cpp/pull/139#issuecomment-1396387216 > I have left the last comments. This PR overall LGTM. Please avoid committing such a huge patch next time. If a PR had many code changes, it would be very hard to review all of them. Some potential issues might not be exposed. Get it, Thanks for your professional reviews! -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] Technoboy- commented on issue #19267: backlog quota is not reflecting on topic level
Technoboy- commented on issue #19267: URL: https://github.com/apache/pulsar/issues/19267#issuecomment-1396378853 Could you provide the dump file of the issued 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar-client-cpp] shibd commented on a diff in pull request #157: [feat] Support auto download schema when create producer.
shibd commented on code in PR #157:
URL: https://github.com/apache/pulsar-client-cpp/pull/157#discussion_r1080761256
##
lib/HTTPLookupService.cc:
##
@@ -409,4 +430,72 @@ void
HTTPLookupService::handleLookupHTTPRequest(LookupPromise promise, const std
}
}
+void HTTPLookupService::handleGetSchemaHTTPRequest(GetSchemaPromise promise,
const std::string completeUrl) {
+std::string responseData;
+Result result = sendHTTPRequest(completeUrl, responseData);
+
+if (result == ResultNotFound) {
+promise.setValue(boost::none);
Review Comment:
Get it. I add a overload methods: `Result sendHTTPRequest(std::string
completeUrl, std::string& responseData, long& responseCode);`, PTAL.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar-client-cpp] shibd commented on a diff in pull request #170: [feature] Support set ServiceUrlProvider when create client.
shibd commented on code in PR #170:
URL: https://github.com/apache/pulsar-client-cpp/pull/170#discussion_r1080757958
##
include/pulsar/Client.h:
##
@@ -66,6 +67,31 @@ class PULSAR_PUBLIC Client {
*/
Client(const std::string& serviceUrl, const ClientConfiguration&
clientConfiguration);
+/**
+ * Create a Pulsar client object connecting to the specified cluster
address and using the default
+ * configuration.
+ *
+ * Instead of specifying a static service URL string (with {@link
#serviceUrl(String)}), an application
+ * can pass a {@link ServiceUrlProvider} function that dynamically provide
a service URL.
+ *
+ * @param serviceUrlProvider The serviceUrlProvider used to generate
ServiceUrl.
+ * @throw std::invalid_argument if `serviceUrlProvider()` return a invalid
url.
+ */
+Client(ServiceUrlProvider serviceUrlProvider);
Review Comment:
This request the user to choose between `Client(ServiceUrlProvider
serviceUrlProvider);` and `Client(const std::string& serviceUrl);` constructors.
If add `setServiceUrlProvider` to `ClientConfiguration`. When the user sets
it this way:
```c++
ClientConfiguration config;
config.setServiceUrlProvider(serviceUrlProvider);
Client client(serviceUrl);
```
This will be confuse.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar-client-cpp] BewareMyPower commented on a diff in pull request #170: [feature] Support set ServiceUrlProvider when create client.
BewareMyPower commented on code in PR #170:
URL: https://github.com/apache/pulsar-client-cpp/pull/170#discussion_r1080755872
##
lib/ServiceNameResolver.h:
##
@@ -29,29 +29,33 @@ namespace pulsar {
class ServiceNameResolver {
public:
ServiceNameResolver(const std::string& uriString)
-: serviceUri_(uriString),
numAddresses_(serviceUri_.getServiceHosts().size()) {
+: serviceUri_(std::make_shared(uriString)),
+ numAddresses_(serviceUri_->getServiceHosts().size()) {
assert(numAddresses_ > 0); // the validation has been done in
ServiceURI
}
ServiceNameResolver(const ServiceNameResolver&) = delete;
ServiceNameResolver& operator=(const ServiceNameResolver&) = delete;
bool useTls() const noexcept {
-return serviceUri_.getScheme() == PulsarScheme::PULSAR_SSL ||
- serviceUri_.getScheme() == PulsarScheme::HTTPS;
+return serviceUri_->getScheme() == PulsarScheme::PULSAR_SSL ||
+ serviceUri_->getScheme() == PulsarScheme::HTTPS;
}
bool useHttp() const noexcept {
-return serviceUri_.getScheme() == PulsarScheme::HTTP ||
- serviceUri_.getScheme() == PulsarScheme::HTTPS;
+return serviceUri_->getScheme() == PulsarScheme::HTTP ||
+ serviceUri_->getScheme() == PulsarScheme::HTTPS;
}
const std::string& resolveHost() {
-return serviceUri_.getServiceHosts()[(numAddresses_ == 1) ? 0 :
(index_++ % numAddresses_)];
+return serviceUri_->getServiceHosts()[(numAddresses_ == 1) ? 0 :
(index_++ % numAddresses_)];
}
+void updateServiceUrl(const std::string& urlString) {
serviceUri_.reset(new ServiceURI(urlString)); }
Review Comment:
There is a problem that if the `urlString` contains multiple service URLs,
we have to update `numAddresses_` as well. A possible solution is moving
`numAddresses_` into `ServiceURI`.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar-client-cpp] BewareMyPower commented on a diff in pull request #170: [feature] Support set ServiceUrlProvider when create client.
BewareMyPower commented on code in PR #170:
URL: https://github.com/apache/pulsar-client-cpp/pull/170#discussion_r1080753416
##
lib/ClientImpl.cc:
##
@@ -497,6 +497,20 @@ void ClientImpl::getPartitionsForTopicAsync(const
std::string& topic, GetPartiti
std::placeholders::_2, topicName, callback));
}
+Result ClientImpl::updateServiceUrl(const std::string& serviceUrl) {
+LOG_INFO("Updating service URL to " << serviceUrl);
+
+try {
+serviceNameResolver_.updateServiceUrl(serviceUrl);
+lookupServicePtr_->updateServiceUrl(serviceUrl);
Review Comment:
Actually we don't need to add `updateServiceUrl` method to `LookupService`
because in C++ client, all `LookupService` implementations just call
`serviceNameResolver_.updateServiceUrl(serviceUrl);`.
Unlike Java client, `HttpLookupService` uses a `HttpClient` class that
accepts a service URL.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] lightistor commented on issue #13965: Flakiness issues with subscriptionKeySharedUseConsistentHashing=true / PIP-119 in CPP tests
lightistor commented on issue #13965: URL: https://github.com/apache/pulsar/issues/13965#issuecomment-1396365880 We are facing this issue that yields a constantly increasing backlog with millions of messages. Is there a fix for it or a guidance to implement our consumers? We end-up with 2 out of 16 consumer service pods that can subscribe with the others idling and not receiving any message. -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar-client-cpp] BewareMyPower commented on a diff in pull request #170: [feature] Support set ServiceUrlProvider when create client.
BewareMyPower commented on code in PR #170:
URL: https://github.com/apache/pulsar-client-cpp/pull/170#discussion_r1080751592
##
lib/ClientImpl.cc:
##
@@ -497,6 +497,20 @@ void ClientImpl::getPartitionsForTopicAsync(const
std::string& topic, GetPartiti
std::placeholders::_2, topicName, callback));
}
+Result ClientImpl::updateServiceUrl(const std::string& serviceUrl) {
+LOG_INFO("Updating service URL to " << serviceUrl);
+
+try {
+serviceNameResolver_.updateServiceUrl(serviceUrl);
+lookupServicePtr_->updateServiceUrl(serviceUrl);
+} catch (const std::exception& e) {
Review Comment:
Catch `std::invalid_argument` instead of `std::exception`
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar-client-cpp] BewareMyPower commented on a diff in pull request #170: [feature] Support set ServiceUrlProvider when create client.
BewareMyPower commented on code in PR #170:
URL: https://github.com/apache/pulsar-client-cpp/pull/170#discussion_r1080749572
##
lib/ServiceNameResolver.h:
##
@@ -29,29 +29,33 @@ namespace pulsar {
class ServiceNameResolver {
Review Comment:
Add a `#include ` to this file for `std::make_shared`. The
compilation did not fail because the `.cc` files that include this header
already include ``.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar-client-cpp] BewareMyPower commented on a diff in pull request #170: [feature] Support set ServiceUrlProvider when create client.
BewareMyPower commented on code in PR #170:
URL: https://github.com/apache/pulsar-client-cpp/pull/170#discussion_r1080746462
##
include/pulsar/Client.h:
##
@@ -66,6 +67,31 @@ class PULSAR_PUBLIC Client {
*/
Client(const std::string& serviceUrl, const ClientConfiguration&
clientConfiguration);
+/**
+ * Create a Pulsar client object connecting to the specified cluster
address and using the default
+ * configuration.
+ *
+ * Instead of specifying a static service URL string (with {@link
#serviceUrl(String)}), an application
+ * can pass a {@link ServiceUrlProvider} function that dynamically provide
a service URL.
+ *
+ * @param serviceUrlProvider The serviceUrlProvider used to generate
ServiceUrl.
+ * @throw std::invalid_argument if `serviceUrlProvider()` return a invalid
url.
+ */
+Client(ServiceUrlProvider serviceUrlProvider);
Review Comment:
Instead of adding new constructor overloads, we should add
`setServiceUrlProvider` to `ClientConfiguration`.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] 315157973 commented on pull request #19094: [feat][admin] PIP-219 Part-1 Add admin API for trimming topic
315157973 commented on PR #19094: URL: https://github.com/apache/pulsar/pull/19094#issuecomment-1396350234 @codelipenghui PTAL -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #18390: [fix][broker] fix bug caused by optimistic locking
github-actions[bot] commented on PR #18390: URL: https://github.com/apache/pulsar/pull/18390#issuecomment-1396346561 The pr had no activity for 30 days, mark with Stale label. -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #18886: [fix][broker] Fix PendingAckHandleImpl when replay failed.
github-actions[bot] commented on PR #18886: URL: https://github.com/apache/pulsar/pull/18886#issuecomment-1396346055 The pr had no activity for 30 days, mark with Stale label. -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on issue #18950: PIP-229: Add a common interface to get fields of the MessageIdData
github-actions[bot] commented on issue #18950: URL: https://github.com/apache/pulsar/issues/18950#issuecomment-1396345955 The issue had no activity for 30 days, mark with Stale label. -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on issue #18957: PIP-230: Throw exception when MessageIdImpl and BatchMessageIdImpl compare with each other
github-actions[bot] commented on issue #18957: URL: https://github.com/apache/pulsar/issues/18957#issuecomment-1396345925 The issue had no activity for 30 days, mark with Stale label. -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on issue #18963: Add metrics for failed topic load operation
github-actions[bot] commented on issue #18963: URL: https://github.com/apache/pulsar/issues/18963#issuecomment-1396345865 The issue had no activity for 30 days, mark with Stale label. -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #18978: Fix consumer stuck when disconnect with broker
github-actions[bot] commented on PR #18978: URL: https://github.com/apache/pulsar/pull/18978#issuecomment-1396345824 The pr had no activity for 30 days, mark with Stale label. -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] Demogorgon314 commented on pull request #18777: [improve][broker] PIP-192: Implement load data store
Demogorgon314 commented on PR #18777: URL: https://github.com/apache/pulsar/pull/18777#issuecomment-1396342667 /pulsarbot run-failure-checks -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar-client-cpp] shibd commented on a diff in pull request #157: [feat] Support auto download schema when create producer.
shibd commented on code in PR #157:
URL: https://github.com/apache/pulsar-client-cpp/pull/157#discussion_r1080735293
##
tests/ClientTest.cc:
##
@@ -305,6 +305,7 @@ TEST(ClientTest, testCloseClient) {
auto t0 = std::chrono::steady_clock::now();
while ((std::chrono::steady_clock::now() - t0) <
std::chrono::microseconds(i)) {
}
+sleep(1);
Review Comment:
Oh, sorry. This was written by my local test and I submitted it by mistake.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[pulsar-site] branch main updated: Docs sync done from apache/pulsar (#ea4f7eb)
This is an automated email from the ASF dual-hosted git repository. urfree pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/pulsar-site.git The following commit(s) were added to refs/heads/main by this push: new 879d1f73d52 Docs sync done from apache/pulsar (#ea4f7eb) 879d1f73d52 is described below commit 879d1f73d52914d925692be325f67cceaa7a22b8 Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> AuthorDate: Thu Jan 19 01:39:18 2023 + Docs sync done from apache/pulsar (#ea4f7eb) --- .../next/config/reference-configuration-broker.md | 33 -- .../config/reference-configuration-pulsar-proxy.md | 33 -- .../config/reference-configuration-standalone.md | 33 -- static/swagger/master/swagger.json | 39 -- static/swagger/master/v2/swagger.json | 39 -- 5 files changed, 108 insertions(+), 69 deletions(-) diff --git a/static/reference/next/config/reference-configuration-broker.md b/static/reference/next/config/reference-configuration-broker.md index a115d9fa13a..2ca37163a64 100644 --- a/static/reference/next/config/reference-configuration-broker.md +++ b/static/reference/next/config/reference-configuration-broker.md @@ -3118,6 +3118,17 @@ Interval between checks to see if message publish buffer size is exceed the max **Category**: Server +### metadataStoreAllowReadOnlyOperations +Is metadata store read-only operations. + +**Type**: `boolean` + +**Default**: `false` + +**Dynamic**: `false` + +**Category**: Server + ### metadataStoreBatchingEnabled Whether we should enable metadata operations batching @@ -3597,17 +3608,6 @@ Specify the TLS provider for the web service: SunJSSE, Conscrypt and etc. **Category**: Server -### zooKeeperAllowReadOnlyOperations -Is zookeeper allow read-only operations. - -**Type**: `boolean` - -**Default**: `false` - -**Dynamic**: `false` - -**Category**: Server - ### bookkeeperClientAuthenticationParameters Parameters for bookkeeper auth plugin @@ -5159,6 +5159,17 @@ Global Zookeeper quorum connection string (as a comma-separated list). Deprecate **Category**: Server +### zooKeeperAllowReadOnlyOperations +Is zookeeper allow read-only operations. + +**Type**: `boolean` + +**Default**: `false` + +**Dynamic**: `false` + +**Category**: Server + ### zooKeeperCacheExpirySeconds ZooKeeper cache expiry time in seconds. @deprecated - Use metadataStoreCacheExpirySeconds instead. diff --git a/static/reference/next/config/reference-configuration-pulsar-proxy.md b/static/reference/next/config/reference-configuration-pulsar-proxy.md index f2460e9a092..567909312ef 100644 --- a/static/reference/next/config/reference-configuration-pulsar-proxy.md +++ b/static/reference/next/config/reference-configuration-pulsar-proxy.md @@ -785,6 +785,17 @@ Max size of messages. **Category**: Server +### metadataStoreAllowReadOnlyOperations +Is metadata store read-only operations. + +**Type**: `boolean` + +**Default**: `false` + +**Dynamic**: `false` + +**Category**: Server + ### metadataStoreCacheExpirySeconds Metadata store cache expiry time in seconds. @@ -917,17 +928,6 @@ The port for serving https requests **Category**: Server -### zooKeeperAllowReadOnlyOperations -Is zooKeeper allow read-only operations. - -**Type**: `boolean` - -**Default**: `false` - -**Dynamic**: `false` - -**Category**: Server - ### tlsAllowInsecureConnection Accept untrusted TLS certificate from client. @@ -1218,4 +1218,15 @@ ZooKeeper session timeout in milliseconds. @deprecated - Use metadataStoreSessio **Category**: Broker Discovery +### zooKeeperAllowReadOnlyOperations +Is zooKeeper allow read-only operations. + +**Type**: `boolean` + +**Default**: `false` + +**Dynamic**: `false` + +**Category**: Server + diff --git a/static/reference/next/config/reference-configuration-standalone.md b/static/reference/next/config/reference-configuration-standalone.md index a115d9fa13a..2ca37163a64 100644 --- a/static/reference/next/config/reference-configuration-standalone.md +++ b/static/reference/next/config/reference-configuration-standalone.md @@ -3118,6 +3118,17 @@ Interval between checks to see if message publish buffer size is exceed the max **Category**: Server +### metadataStoreAllowReadOnlyOperations +Is metadata store read-only operations. + +**Type**: `boolean` + +**Default**: `false` + +**Dynamic**: `false` + +**Category**: Server + ### metadataStoreBatchingEnabled Whether we should enable metadata operations batching @@ -3597,17 +3608,6 @@ Specify the TLS provider for the web service: SunJSSE, Conscrypt and etc. **Category**: Server -### zooKeeperAllowReadOnlyOperations -Is zookeeper allow read-only operations. - -**Type**: `boolean` - -**Default**: `false` - -**Dynamic**: `false` - -**Category**: Server - ### bookkeeperClientAuthenticationParameters Parameters
[GitHub] [pulsar] michaeljmarshall commented on pull request #19280: [improve][broker] AuthenticationState authenticate data once
michaeljmarshall commented on PR #19280: URL: https://github.com/apache/pulsar/pull/19280#issuecomment-1396224344 After thinking about this more, I think we should probably take a different direction. I'll follow up with another 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] michaeljmarshall closed pull request #19280: [improve][broker] AuthenticationState authenticate data once
michaeljmarshall closed pull request #19280: [improve][broker] AuthenticationState authenticate data once URL: https://github.com/apache/pulsar/pull/19280 -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] codecov-commenter commented on pull request #19278: [cleanup][proxy] Remove unused AuthenticationDataSource variable
codecov-commenter commented on PR #19278: URL: https://github.com/apache/pulsar/pull/19278#issuecomment-1396211146 # [Codecov](https://codecov.io/gh/apache/pulsar/pull/19278?src=pr=h1_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) Report > Merging [#19278](https://codecov.io/gh/apache/pulsar/pull/19278?src=pr=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) (2ae7fdc) into [master](https://codecov.io/gh/apache/pulsar/commit/334c3a541f00c08aa2f378a6cc45f37cbeb54a5c?el=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) (334c3a5) will **decrease** coverage by `15.97%`. > The diff coverage is `23.65%`. [](https://codecov.io/gh/apache/pulsar/pull/19278?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) ```diff @@ Coverage Diff @@ ## master #19278 +/- ## = - Coverage 47.04% 31.07% -15.97% - Complexity 919012372 +3182 = Files 607 1575 +968 Lines 57677 123863+66186 Branches 600713486 +7479 = + Hits 2713238493+11361 - Misses2759880761+53163 - Partials 2947 4609 +1662 ``` | Flag | Coverage Δ | | |---|---|---| | inttests | `20.22% <6.63%> (?)` | | | unittests | `51.26% <21.46%> (+4.22%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/apache/pulsar/pull/19278?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation) | Coverage Δ | | |---|---|---| | [...rg/apache/pulsar/broker/delayed/bucket/Bucket.java](https://codecov.io/gh/apache/pulsar/pull/19278?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9kZWxheWVkL2J1Y2tldC9CdWNrZXQuamF2YQ==) | `0.00% <0.00%> (ø)` | | | [...r/delayed/bucket/BucketDelayedDeliveryTracker.java](https://codecov.io/gh/apache/pulsar/pull/19278?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9kZWxheWVkL2J1Y2tldC9CdWNrZXREZWxheWVkRGVsaXZlcnlUcmFja2VyLmphdmE=) | `0.00% <0.00%> (ø)` | | | [...layed/bucket/CombinedSegmentDelayedIndexQueue.java](https://codecov.io/gh/apache/pulsar/pull/19278?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9kZWxheWVkL2J1Y2tldC9Db21iaW5lZFNlZ21lbnREZWxheWVkSW5kZXhRdWV1ZS5qYXZh) | `0.00% <0.00%> (ø)` | | | [...ulsar/broker/delayed/bucket/DelayedIndexQueue.java](https://codecov.io/gh/apache/pulsar/pull/19278?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9kZWxheWVkL2J1Y2tldC9EZWxheWVkSW5kZXhRdWV1ZS5qYXZh) | `0.00% <0.00%> (ø)` | | | [.../pulsar/broker/delayed/bucket/ImmutableBucket.java](https://codecov.io/gh/apache/pulsar/pull/19278?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9kZWxheWVkL2J1Y2tldC9JbW11dGFibGVCdWNrZXQuamF2YQ==) | `0.00% <0.00%> (ø)` | | | [...he/pulsar/broker/delayed/bucket/MutableBucket.java](https://codecov.io/gh/apache/pulsar/pull/19278?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9kZWxheWVkL2J1Y2tldC9NdXRhYmxlQnVja2V0LmphdmE=) | `0.00% <0.00%> (ø)` | | |
[GitHub] [pulsar] michaeljmarshall opened a new pull request, #19280: [improve][broker] AuthenticationState authenticate data once
michaeljmarshall opened a new pull request, #19280:
URL: https://github.com/apache/pulsar/pull/19280
PIP 97: https://github.com/apache/pulsar/issues/12105
### Motivation
In order to implement PIP 97, it is essential that we remove all blocking
calls to `provider.authenticate(authData)`. Two places where we currently call
the synchronous `authenticate` method are in the `OneStageAuthenticationState`
and the `TokenAuthenticationState` class constructors. Since `authenticate`
will be replaced with `authenticateAsync`, we won't be able to rely on
authenticating the `authData` in the constructor. Further, it is inefficient to
verify authentication data twice, which currently happens for
`TokenAuthenticationState` in the `ProxyConnection` and the `ServerCnx`.
This PR seeks to make explicit a paradigm that is already followed in the
Pulsar code base. Specifically, that the contract for using an
`AuthenticationState` class is as follows:
1. Initializing the class by calling {@link
AuthenticationProvider#newAuthState(SocketAddress, SSLSession)}
2. Calling {@link #authenticate(AuthData)} in a loop until the result is
null.
3. Calling {@link #getAuthRole()} and {@link #getAuthDataSource()} to use
for authentication.
4. Calling {@link #refreshAuthentication()} when {@link #isExpired()}
returns true.
5. GoTo step 3.
When PIP 97 is complete, step 2 will replace `authenticate` with
`authenticateAsync`.
### Modifications
* Add `AuthenticationProvider` method: `newAuthState(SocketAddress
remoteAddress, SSLSession sslSession)`. This method does not take `AuthData` to
make it a bit clearer that class constructors are not meant to perform
authentication. This method's default definition is
`OneStageAuthenticationState`. Users that want a custom
`AuthenticationProvider` to use a different `AuthenticationState`
implementation must override this method.
* Add `@Deprecated` annotation to `newAuthState(AuthData authData,
SocketAddress remoteAddress, SSLSession sslSession)` and to several class
constructors that were used by that method. Note: leave the implementations the
same for broker extensions that use this method. This is the only backwards
compatible portion of this PR.
* Update `ProxyConnection` and `ServerCnx` to use the new `newAuthState`
method. This change could break custom `AuthenticationState` implementations as
described in the first bullet point.
* Update `TokenAuthenticationState` and `OneStageAuthenticationState` to
only authenticate tokens in the `authenticate` method.
### Verifying this change
There are new tests and there is already some test coverage.
### Does this pull request potentially affect one of the following parts:
- [x] The public API
This change affects the `AuthenticationProvider` interface.
### Documentation
- [x] `doc-not-needed`
We document the authentication interfaces in the code, and I've added docs
there.
### Matching PR in forked repository
PR in forked repository: https://github.com/michaeljmarshall/pulsar/pull/14
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] pgier opened a new pull request, #19279: [improve][cli] pulsar-perf: refactor to reduce code duplication
pgier opened a new pull request, #19279: URL: https://github.com/apache/pulsar/pull/19279 ### Motivation Some of the code in the pulsar-perf command line tools seems to be unnecessarily repetitive. ### Modifications This removes some code duplication in the pulsar-perf utilities by pushing the common CLI parsing logic up into the shared `PerformanceBaseArguments` class. This also deprecates some inconsistent CLI arg naming, and make some improvements to the CLI arg validation. ### Verifying this change - [X] Make sure that the change passes the CI checks. This change is already covered by existing tests, such as the pulsar-perf tests. ### 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` - [X] `doc-not-needed` - [ ] `doc-complete` ### Matching PR in forked repository PR in forked repository: https://github.com/pgier/pulsar/pull/8 -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] frankjkelly commented on issue #16249: Pulsar Admin API client should support retries on operations
frankjkelly commented on issue #16249:
URL: https://github.com/apache/pulsar/issues/16249#issuecomment-1396150858
@martijngonlag Sorry I took so long to get back to you.
The problem then becomes our code is littered with the following since some
of the exceptions are not retryable and others are. And then we have to catch
this further up and (using Spring) do the retries. It's an awful lot of
boilerplate and particularly problematic for customers like us that create and
delete a lot of topics. Especially with ZK being such a non-performant beast
(and involved on the create/delete topic flow).
```
private void createTopic(final URI topic) throws
StreamManagerAdminException {
try {
val start = System.currentTimeMillis();
pulsarAdmin.topics().createNonPartitionedTopic(topic.toString());
val durationMs = System.currentTimeMillis() - start;
recordDurationMetric("createTopic", durationMs);
} catch (final PulsarAdminException e) {
castAndRethrowException(e);
}
}
private void castAndRethrowException(final PulsarAdminException e)
throws StreamManagerAdminException {
//Need to be able to distinguish between exceptions to be able to
know which are retryable or not
// However we do not want to expose Pulsar Internals beyond this
layer
if (e.getStatusCode() == 503) {
throw new StreamManagerServiceUnavailableException(e);
} else if (didPulsarAdminExhaustRetries(e)) {
throw new StreamManagerRetryException(e);
} else {
throw new StreamManagerAdminException(e);
}
}
```
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] michaeljmarshall opened a new pull request, #19278: [cleanup][proxy] Remove unused AuthenticationDataSource variable
michaeljmarshall opened a new pull request, #19278: URL: https://github.com/apache/pulsar/pull/19278 ### Motivation The `AuthenticationDataSource` variable was added to the `ProxyConnection` class in https://github.com/apache/pulsar/pull/1200. It is no longer needed. ### Modifications * Remove unused variable ### Verifying this change This is a trivial change. ### Does this pull request potentially affect one of the following parts: The only conceivable way this is a breaking change is if a third party implementation implemented their library so that `authState.getAuthDataSource()` has a side effect. ### Documentation - [x] `doc-not-needed` No docs needed. -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #19270: [cleanup][broker] Validate originalPrincipal earlier in ServerCnx
michaeljmarshall commented on code in PR #19270:
URL: https://github.com/apache/pulsar/pull/19270#discussion_r1073983457
##
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java:
##
@@ -435,6 +435,55 @@ public void testConnectCommandWithAuthenticationPositive()
throws Exception {
channel.finish();
}
+@Test(timeOut = 3)
+public void testConnectCommandWithInvalidRoleCombinations() throws
Exception {
+AuthenticationService authenticationService =
mock(AuthenticationService.class);
+AuthenticationProvider authenticationProvider = new
BasicCommandAuthenticationProvider();
+String authMethodName = authenticationProvider.getAuthMethodName();
+
+
when(brokerService.getAuthenticationService()).thenReturn(authenticationService);
+
when(authenticationService.getAuthenticationProvider(authMethodName)).thenReturn(authenticationProvider);
+svcConfig.setAuthenticationEnabled(true);
+svcConfig.setAuthorizationEnabled(true);
+svcConfig.setProxyRoles(Collections.singleton("proxy"));
+
+resetChannel();
+assertTrue(channel.isActive());
+assertEquals(serverCnx.getState(), State.Start);
+
+// Invalid combinations where authData is proxy role
+verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "proxy",
"proxy");
+verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "proxy",
"");
+verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "proxy",
null);
+// Invalid combinations where original principal is set to a proxy role
+verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "",
"proxy");
+verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, null,
"proxy");
+verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "client",
"proxy");
+// Invalid combinations where the original principal is set to a
non-proxy role
+verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "",
"some_user");
+verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, null,
"some_user");
+verifyAuthRoleAndOriginalPrincipalBehavior(authMethodName, "client",
"some_user");
+}
+
+private void verifyAuthRoleAndOriginalPrincipalBehavior(String
authMethodName, String authData,
+String
originalPrincipal) throws Exception {
+resetChannel();
+assertTrue(channel.isActive());
+assertEquals(serverCnx.getState(), State.Start);
+
+ByteBuf clientCommand = Commands.newConnect(authMethodName, authData,
1,null,
+null, originalPrincipal, null, null);
+channel.writeInbound(clientCommand);
+
+Object response1 = getResponse();
+assertTrue(response1 instanceof CommandConnected);
Review Comment:
In order to get this test to pass, I needed to add this verification because
we send the response first. It'd be ideal to only send the `CommandError`, if
possible.
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##
@@ -965,15 +961,23 @@ protected void handleConnect(CommandConnect connect) {
remoteAddress, originalPrincipal);
}
}
+if (service.isAuthorizationEnabled()) {
+validateRoleAndOriginalPrincipal();
+}
Review Comment:
It's not always correct to verify this here. Specifically, the
authentication might not be complete yet in the case of SASL.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] andrasbeni commented on pull request #19208: [improve][broker]Enable custom metadata stores
andrasbeni commented on PR #19208: URL: https://github.com/apache/pulsar/pull/19208#issuecomment-1387544316 /pulsarbot rerun-failure-checks -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] poorbarcode opened a new pull request, #19275: [fix] [ml] Topics stats shows msgBacklog but there reality no backlog
poorbarcode opened a new pull request, #19275:
URL: https://github.com/apache/pulsar/pull/19275
### Motivation
When `trim ledgers` and `create new cursor` are executed concurrently, it
will cause the `readPosition` of the cursor to point to a deleted ledger.
| time | `trim ledgers` | `create new cursor` |
| --- | --- | --- |
| 1 | | set read position and mark deleted position |
| 2 | delete ledger | |
| 3 | | add the cursor to `ManagedLedger.cursors` |
(Highlight)Since the read position of the cursor is
pointing at a deleted ledger, so deleted messages will never be consumed or
acknowledged. Since the backlog in the API `topics stats` response is
calculated as this: `managedLedger.entriesAddedCounter -
cursor.messagesConsumedCounter`, the result is: Topics stats show `msgBacklog`
but there is reality no backlog.
- `managedLedger.entriesAddedCounter`: Pulsar will set it to `0` when
creating a new managed ledger, it will increment when adding entries.
- `cursor.messagesConsumedCounter`: Pulsar will set it to `0` when creating
a new cursor, it will increment when acknowledging.
For example:
- write entries to the managed ledger: `{1:0~1:10}...{5:0~5:10}`
- `managedLedger.entriesAddedCounter` is `50` now
- create a new cursor, and set the read position to `1:0`
- `managedLedger.entriesAddedCounter` is `0` now
- delete ledgers `1~4`
- consume all messages
- can only consume the messages {5:0~5:10}, so
`managedLedger.entriesAddedCounter` is `5` now
- the `backlog` in response of `topics stats` is `50 - 5 = 45`, but there
reality no backlog
Sorry, I spent 4 hours trying to write a non-invasive test, but failed.
(Highlight)You can reproduce by
`testBacklogIfCursorCreateConcurrentWithTrimLedger` in the PR #19274
https://github.com/apache/pulsar/blob/a2cdc759fc2710e4dd913eb0485d23ebcaa076a4/pulsar-broker/src/test/java/org/apache/bookkeeper/mledger/impl/StatsBackLogTest.java#L163
### Modifications
If there are uninitialized cursors, execution `trim ledgers` is deferred.
### Documentation
- [ ] `doc`
- [ ] `doc-required`
- [x] `doc-not-needed`
- [ ] `doc-complete`
### Matching PR in forked repository
PR in forked repository:
- https://github.com/poorbarcode/pulsar/pull/53
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #18130: [fix][broker] Fix update authentication data
michaeljmarshall commented on code in PR #18130:
URL: https://github.com/apache/pulsar/pull/18130#discussion_r1073892338
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##
@@ -707,73 +709,96 @@ private void completeConnect(int clientProtoVersion,
String clientVersion, boole
}
// According to auth result, send newConnected or newAuthChallenge command.
-private State doAuthentication(AuthData clientData,
- int clientProtocolVersion,
- String clientVersion) throws Exception {
-
+private CompletableFuture doAuthenticationAsync(AuthData clientData,
int clientProtocolVersion,
+ String
clientVersion) {
// The original auth state can only be set on subsequent auth attempts
(and only
// in presence of a proxy and if the proxy is forwarding the
credentials).
// In this case, the re-validation needs to be done against the
original client
-// credentials.
-boolean useOriginalAuthState = (originalAuthState != null);
-AuthenticationState authState = useOriginalAuthState ?
originalAuthState : this.authState;
-String authRole = useOriginalAuthState ? originalPrincipal :
this.authRole;
-AuthData brokerData = authState.authenticate(clientData);
-
-if (log.isDebugEnabled()) {
-log.debug("Authenticate using original auth state : {}, role =
{}", useOriginalAuthState, authRole);
-}
+// credentials, but we only can new an authentication state, because
some authentication data(TLS, SASL)
+// based on outside service.
+// If we can get the role from the authentication sate, the global
variable need to be updated.
-if (authState.isComplete()) {
-// Authentication has completed. It was either:
-// 1. the 1st time the authentication process was done, in which
case we'll send
-//a `CommandConnected` response
-// 2. an authentication refresh, in which case we need to refresh
authenticationData
-
-String newAuthRole = authState.getAuthRole();
-
-// Refresh the auth data.
-this.authenticationData = authState.getAuthDataSource();
+boolean useOriginalAuthState = (originalAuthState != null);
+if (state == State.Connected) {
+// For auth challenge, the authentication state requires to be
updated.
if (log.isDebugEnabled()) {
-log.debug("[{}] Auth data refreshed for role={}",
remoteAddress, this.authRole);
+log.debug("Refreshing authenticate state, original auth state:
{}, original auth role: {}, "
++ "auth role: {}",
+useOriginalAuthState, originalPrincipal, authRole);
}
-
-if (!useOriginalAuthState) {
-this.authRole = newAuthRole;
+try {
+if (useOriginalAuthState) {
+originalAuthState =
+
originalAuthenticationProvider.newAuthState(clientData, remoteAddress,
sslSession);
+} else {
+authState =
authenticationProvider.newAuthState(clientData, remoteAddress, sslSession);
Review Comment:
I think the underlying problem is that calling `authenticate` does not
update the `AuthenticationDataSource` in our token authentication provider. One
solution is to recreate an auth state object. However, this has not been the
implicit contract in the `ServerCnx` for as long as we've had the
`AuthenticationState`. The contract has been that the `AuthenticationState`
lasts the whole life of the connection. I think we should update the
`TokenAuthenticationState` so that `authenticate` updates the
`AuthenticationDataSource`.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #18130: [fix][broker] Fix update authentication data
michaeljmarshall commented on code in PR #18130:
URL: https://github.com/apache/pulsar/pull/18130#discussion_r1073882511
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##
@@ -707,73 +709,96 @@ private void completeConnect(int clientProtoVersion,
String clientVersion, boole
}
// According to auth result, send newConnected or newAuthChallenge command.
-private State doAuthentication(AuthData clientData,
- int clientProtocolVersion,
- String clientVersion) throws Exception {
-
+private CompletableFuture doAuthenticationAsync(AuthData clientData,
int clientProtocolVersion,
+ String
clientVersion) {
// The original auth state can only be set on subsequent auth attempts
(and only
// in presence of a proxy and if the proxy is forwarding the
credentials).
// In this case, the re-validation needs to be done against the
original client
-// credentials.
-boolean useOriginalAuthState = (originalAuthState != null);
-AuthenticationState authState = useOriginalAuthState ?
originalAuthState : this.authState;
-String authRole = useOriginalAuthState ? originalPrincipal :
this.authRole;
-AuthData brokerData = authState.authenticate(clientData);
-
-if (log.isDebugEnabled()) {
-log.debug("Authenticate using original auth state : {}, role =
{}", useOriginalAuthState, authRole);
-}
+// credentials, but we only can new an authentication state, because
some authentication data(TLS, SASL)
+// based on outside service.
Review Comment:
> For `originalAuthentication`, we don't call the authentication checks.
Isn't this a problem though? We aren't really authenticating the
`originalAuthData` if we don't call the `authenticate` method and make sure
authentication is "complete". In the `ProxyConnectionToBroker` case, we can
send back `AuthChallenge In the event that the proxy is forwarding
authentication information, we can issue `AuthChallenge` responses. It might
not work so easily in the `ProxyLookupRequests` state.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #19274: [Do not merge][Issue] Reproduce:Topics stats shows msgBacklog but there reality no backlog
github-actions[bot] commented on PR #19274: URL: https://github.com/apache/pulsar/pull/19274#issuecomment-1387481501 @poorbarcode Please add the following content to your PR description and select a checkbox: ``` - [ ] `doc` - [ ] `doc-required` - [ ] `doc-not-needed` - [ ] `doc-complete` ``` -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] poorbarcode opened a new pull request, #19274: [Do not merge][Issue] Reproduce:Topics stats shows msgBacklog but there reality no backlog
poorbarcode opened a new pull request, #19274: URL: https://github.com/apache/pulsar/pull/19274 ### Issue -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] nlu90 commented on issue #19123: PIP-237: Make PulsarAdmin accessible in SinkContext and SourceContext
nlu90 commented on issue #19123: URL: https://github.com/apache/pulsar/issues/19123#issuecomment-1387478830 +1 for option b. One note is that there's a switch to enable the access of PulsarAdmin. We should still keep the switch for SouceContext and SinkContext. -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #18130: [fix][broker] Fix update authentication data
michaeljmarshall commented on code in PR #18130:
URL: https://github.com/apache/pulsar/pull/18130#discussion_r1073869123
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##
@@ -707,73 +709,96 @@ private void completeConnect(int clientProtoVersion,
String clientVersion, boole
}
// According to auth result, send newConnected or newAuthChallenge command.
-private State doAuthentication(AuthData clientData,
- int clientProtocolVersion,
- String clientVersion) throws Exception {
-
+private CompletableFuture doAuthenticationAsync(AuthData clientData,
int clientProtocolVersion,
+ String
clientVersion) {
// The original auth state can only be set on subsequent auth attempts
(and only
// in presence of a proxy and if the proxy is forwarding the
credentials).
// In this case, the re-validation needs to be done against the
original client
-// credentials.
-boolean useOriginalAuthState = (originalAuthState != null);
-AuthenticationState authState = useOriginalAuthState ?
originalAuthState : this.authState;
-String authRole = useOriginalAuthState ? originalPrincipal :
this.authRole;
-AuthData brokerData = authState.authenticate(clientData);
-
-if (log.isDebugEnabled()) {
-log.debug("Authenticate using original auth state : {}, role =
{}", useOriginalAuthState, authRole);
-}
+// credentials, but we only can new an authentication state, because
some authentication data(TLS, SASL)
+// based on outside service.
+// If we can get the role from the authentication sate, the global
variable need to be updated.
-if (authState.isComplete()) {
-// Authentication has completed. It was either:
-// 1. the 1st time the authentication process was done, in which
case we'll send
-//a `CommandConnected` response
-// 2. an authentication refresh, in which case we need to refresh
authenticationData
-
-String newAuthRole = authState.getAuthRole();
-
-// Refresh the auth data.
-this.authenticationData = authState.getAuthDataSource();
Review Comment:
> No. We must check both the `originalAuthState` and `authState`, if you
don't check, you will overstep your authority.
I agree that we currently check authorization on the proxy role and the
original principal as well as with the proxy's `authenticationDataSource` and
the `originalAuthenticationDataSource`. However, I am not sure why that is
necessary, and as we've discussed, this is not even done correctly for the auth
data source.
What is the case that a client will overstep its authority?
When the proxy is forwarding authentication data, it seems sufficient to
verify that the proxy's authentication data is valid for a proxy as a one time
check during connection initialization. All subsequent auth challenges will go
to the client anyway.
If the proxy is unable to forward authentication data, then the broker will
only have the proxy's authentication data and the `originalPrincipal`.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #19270: [cleanup][broker] Validate originalPrincipal earlier in ServerCnx
michaeljmarshall commented on code in PR #19270:
URL: https://github.com/apache/pulsar/pull/19270#discussion_r1073818425
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##
@@ -395,17 +395,32 @@ public void exceptionCaught(ChannelHandlerContext ctx,
Throwable cause) throws E
ctx.close();
}
-/*
- * If authentication and authorization is enabled(and not sasl) and
- * if the authRole is one of proxyRoles we want to enforce
- * - the originalPrincipal is given while connecting
- * - originalPrincipal is not blank
- * - originalPrincipal is not a proxy principal
+/**
+ * When transitioning from Connecting to Connected, this class validates
that the {@link #authRole} and the
+ * {@link #originalPrincipal} are a valid combination. Valid combinations
fulfill the following rule:
+ *
+ * The {@link #authRole} is in {@link #proxyRoles}, if, and only if, the
{@link #originalPrincipal} is set to a role
+ * that is not also in {@link #proxyRoles}.
*/
-private boolean invalidOriginalPrincipal(String originalPrincipal) {
-return (service.isAuthenticationEnabled() &&
service.isAuthorizationEnabled()
-&& proxyRoles.contains(authRole) &&
(StringUtils.isBlank(originalPrincipal)
-|| proxyRoles.contains(originalPrincipal)));
+private void validateRoleAndOriginalPrincipal() {
Review Comment:
> one note is the previous code perform the checks only if authorization is
also enabled, but I think it was wrong since these checks are about
authentication
Good point. The code path checks for authentication but not authorization.
I'll fix that. (As an aside, I wonder who is running with authentication and
without authorization.)
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #19270: [cleanup][broker] Validate originalPrincipal earlier in ServerCnx
michaeljmarshall commented on code in PR #19270:
URL: https://github.com/apache/pulsar/pull/19270#discussion_r1073824302
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##
@@ -395,17 +395,32 @@ public void exceptionCaught(ChannelHandlerContext ctx,
Throwable cause) throws E
ctx.close();
}
-/*
- * If authentication and authorization is enabled(and not sasl) and
- * if the authRole is one of proxyRoles we want to enforce
- * - the originalPrincipal is given while connecting
- * - originalPrincipal is not blank
- * - originalPrincipal is not a proxy principal
+/**
+ * When transitioning from Connecting to Connected, this class validates
that the {@link #authRole} and the
+ * {@link #originalPrincipal} are a valid combination. Valid combinations
fulfill the following rule:
+ *
+ * The {@link #authRole} is in {@link #proxyRoles}, if, and only if, the
{@link #originalPrincipal} is set to a role
+ * that is not also in {@link #proxyRoles}.
*/
-private boolean invalidOriginalPrincipal(String originalPrincipal) {
-return (service.isAuthenticationEnabled() &&
service.isAuthorizationEnabled()
-&& proxyRoles.contains(authRole) &&
(StringUtils.isBlank(originalPrincipal)
-|| proxyRoles.contains(originalPrincipal)));
+private void validateRoleAndOriginalPrincipal() {
+String errorMsg = null;
+if (proxyRoles.contains(authRole)) {
+if (StringUtils.isBlank(originalPrincipal)) {
+errorMsg = "originalPrincipal must be provided when connecting
with a proxy role.";
+} else if (proxyRoles.contains(originalPrincipal)) {
+errorMsg = "originalPrincipal cannot be a proxy role.";
+}
+} else if (StringUtils.isNotBlank(originalPrincipal)) {
+errorMsg = "cannot specify originalPrincipal when connecting
without valid proxy role.";
+}
+if (errorMsg != null) {
+service.getPulsarStats().recordConnectionCreateFail();
Review Comment:
I extracted the code into a shared method so we can avoid an exception and
avoid the code duplication.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #19270: [cleanup][broker] Validate originalPrincipal earlier in ServerCnx
michaeljmarshall commented on code in PR #19270:
URL: https://github.com/apache/pulsar/pull/19270#discussion_r1073817281
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##
@@ -395,17 +395,32 @@ public void exceptionCaught(ChannelHandlerContext ctx,
Throwable cause) throws E
ctx.close();
}
-/*
- * If authentication and authorization is enabled(and not sasl) and
- * if the authRole is one of proxyRoles we want to enforce
- * - the originalPrincipal is given while connecting
- * - originalPrincipal is not blank
- * - originalPrincipal is not a proxy principal
+/**
+ * When transitioning from Connecting to Connected, this class validates
that the {@link #authRole} and the
+ * {@link #originalPrincipal} are a valid combination. Valid combinations
fulfill the following rule:
+ *
+ * The {@link #authRole} is in {@link #proxyRoles}, if, and only if, the
{@link #originalPrincipal} is set to a role
+ * that is not also in {@link #proxyRoles}.
*/
-private boolean invalidOriginalPrincipal(String originalPrincipal) {
-return (service.isAuthenticationEnabled() &&
service.isAuthorizationEnabled()
-&& proxyRoles.contains(authRole) &&
(StringUtils.isBlank(originalPrincipal)
-|| proxyRoles.contains(originalPrincipal)));
+private void validateRoleAndOriginalPrincipal() {
+String errorMsg = null;
+if (proxyRoles.contains(authRole)) {
+if (StringUtils.isBlank(originalPrincipal)) {
+errorMsg = "originalPrincipal must be provided when connecting
with a proxy role.";
+} else if (proxyRoles.contains(originalPrincipal)) {
+errorMsg = "originalPrincipal cannot be a proxy role.";
+}
+} else if (StringUtils.isNotBlank(originalPrincipal)) {
+errorMsg = "cannot specify originalPrincipal when connecting
without valid proxy role.";
+}
+if (errorMsg != null) {
+service.getPulsarStats().recordConnectionCreateFail();
Review Comment:
I considered this, but I decided not to because throwing an exception is
expensive and at this point in the `ServerCnx` lifecycle, I think we should
avoid using exceptions as flow control when possible.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #19270: [cleanup][broker] Validate originalPrincipal earlier in ServerCnx
michaeljmarshall commented on code in PR #19270:
URL: https://github.com/apache/pulsar/pull/19270#discussion_r1073818425
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##
@@ -395,17 +395,32 @@ public void exceptionCaught(ChannelHandlerContext ctx,
Throwable cause) throws E
ctx.close();
}
-/*
- * If authentication and authorization is enabled(and not sasl) and
- * if the authRole is one of proxyRoles we want to enforce
- * - the originalPrincipal is given while connecting
- * - originalPrincipal is not blank
- * - originalPrincipal is not a proxy principal
+/**
+ * When transitioning from Connecting to Connected, this class validates
that the {@link #authRole} and the
+ * {@link #originalPrincipal} are a valid combination. Valid combinations
fulfill the following rule:
+ *
+ * The {@link #authRole} is in {@link #proxyRoles}, if, and only if, the
{@link #originalPrincipal} is set to a role
+ * that is not also in {@link #proxyRoles}.
*/
-private boolean invalidOriginalPrincipal(String originalPrincipal) {
-return (service.isAuthenticationEnabled() &&
service.isAuthorizationEnabled()
-&& proxyRoles.contains(authRole) &&
(StringUtils.isBlank(originalPrincipal)
-|| proxyRoles.contains(originalPrincipal)));
+private void validateRoleAndOriginalPrincipal() {
Review Comment:
> one note is the previous code perform the checks only if authorization is
also enabled, but I think it was wrong since these checks are about
authentication
Fair point. The code path checks for authentication but not authorization.
I'll fix that. (As an aside, I wonder who is running with authentication and
without authorization.)
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #19270: [cleanup][broker] Validate originalPrincipal earlier in ServerCnx
michaeljmarshall commented on code in PR #19270:
URL: https://github.com/apache/pulsar/pull/19270#discussion_r1073817281
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##
@@ -395,17 +395,32 @@ public void exceptionCaught(ChannelHandlerContext ctx,
Throwable cause) throws E
ctx.close();
}
-/*
- * If authentication and authorization is enabled(and not sasl) and
- * if the authRole is one of proxyRoles we want to enforce
- * - the originalPrincipal is given while connecting
- * - originalPrincipal is not blank
- * - originalPrincipal is not a proxy principal
+/**
+ * When transitioning from Connecting to Connected, this class validates
that the {@link #authRole} and the
+ * {@link #originalPrincipal} are a valid combination. Valid combinations
fulfill the following rule:
+ *
+ * The {@link #authRole} is in {@link #proxyRoles}, if, and only if, the
{@link #originalPrincipal} is set to a role
+ * that is not also in {@link #proxyRoles}.
*/
-private boolean invalidOriginalPrincipal(String originalPrincipal) {
-return (service.isAuthenticationEnabled() &&
service.isAuthorizationEnabled()
-&& proxyRoles.contains(authRole) &&
(StringUtils.isBlank(originalPrincipal)
-|| proxyRoles.contains(originalPrincipal)));
+private void validateRoleAndOriginalPrincipal() {
+String errorMsg = null;
+if (proxyRoles.contains(authRole)) {
+if (StringUtils.isBlank(originalPrincipal)) {
+errorMsg = "originalPrincipal must be provided when connecting
with a proxy role.";
+} else if (proxyRoles.contains(originalPrincipal)) {
+errorMsg = "originalPrincipal cannot be a proxy role.";
+}
+} else if (StringUtils.isNotBlank(originalPrincipal)) {
+errorMsg = "cannot specify originalPrincipal when connecting
without valid proxy role.";
+}
+if (errorMsg != null) {
+service.getPulsarStats().recordConnectionCreateFail();
Review Comment:
I considered this, but I decided not to because throwing an exception is
expensive and at this point in the `ServerCnx` lifecycle, I think we should
avoid using exceptions as control flow when possible.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #19197: [feat][broker] Update AuthenticationProvider to simplify HTTP Authn
michaeljmarshall commented on code in PR #19197:
URL: https://github.com/apache/pulsar/pull/19197#discussion_r1073797306
##
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java:
##
@@ -160,6 +163,20 @@ public String authenticate(AuthenticationDataSource
authData) throws Authenticat
}
}
+@Override
+public boolean authenticateHttpRequest(HttpServletRequest request,
HttpServletResponse response) throws Exception {
Review Comment:
My primary motivation for this PR is to implement PIP 97. If we want
authentication to be asynchronous, we cannot assume that when the
`AuthenticationState` object is initialized, the `authenticationDataSource` and
the `authRole` are present because the authentication might not yet be
completed. My goal with this PR is to break an unnecessary relationship between
`AuthenticationState` and http request authentication that was created by
https://github.com/apache/pulsar/pull/14044.
> 1. Using `newHttpAuthState` returns `AuthenticationState`, which includes
role and authentication data, we can simply get these from
`AuthenticationState`, and also quickly check the user authentication data.
This is not the intention of the `newHttpAuthState` method. The method was
added by https://github.com/apache/pulsar/pull/14044 so that the
`AuthenticationDataSource` for an `AuthenticationProvider` can be added on the
http request as the `AuthenticatedDataAttributeName` attribute on the
`request`. This PR proposes an alternate way to meet those criteria so that we
can implement PIP 97.
> 2. Keep the same logic with the `newAuthState`, it looks cleaner.
I disagree that it looks cleaner. The primary reason that we need a state
object in the `pulsar` protocol handlers is that we track `isExpired` and
whether or not the authentication is complete (first with `isComplete` and in
2.12 with the result of the `authenticationAsync`). We do not track state for
individual HTTP request connections, so we shouldn't have a method to create a
state object for HTTP requests.
Further, we're only building the `AuthenticationState` object to get the
`AuthenticationDataSource` and we're not calling the
`AuthenticationState#authenticate` method. There is no need for this
indirection. We can give `AuthenticationProvider` implementations freedom to
add the right `request` attributes by using the `authenticateHttpRequest` (and
eventually `authenticateHttpRequestAsync`).
Also, another problem is with the `AuthenticationDataSource` in the
`AuthenticationState`. I think we should recreate the
`AuthenticationDataSource` each time we call `authenticate`. However, if we
have `newHttpAuthState`, the `AuthenticationState` object is not able to create
a new `AuthenticationDataSource`. I think the `newHttpAuthState` creates an
confusion about how to use the `AuthenticationState`. This is a separate issue
that I will follow up on in another PR.
> I can accept authentication checks in the constructor of
`OneStageAuthenticationState`, it is a quick check.
It does not matter that it is "quick". It is a waste of resources to verify
authentication information twice.
> Maybe we can improve here, but the Pulsar must explicitly call the
`authenticate` of `AuthenticationState`.
This is already the way that Pulsar operates, but it has not been a stated
requirement. I think this is essential for making authentication asynchronous.
I will follow up with a separate 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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] andrasbeni commented on pull request #19208: [improve][broker]Enable custom metadata stores
andrasbeni commented on PR #19208: URL: https://github.com/apache/pulsar/pull/19208#issuecomment-1387319343 /pulsarbot rerun-failure-checks -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar-client-node] BewareMyPower commented on a diff in pull request #288: Make it compatible with GLIBCXX_3.4.19 (CentOS 7)
BewareMyPower commented on code in PR #288: URL: https://github.com/apache/pulsar-client-node/pull/288#discussion_r1073648725 ## .github/workflows/ci-pr-validation.yml: ## @@ -167,6 +167,14 @@ jobs: docker run -i -v $PWD:/pulsar-client-node build:latest \ /pulsar-client-node/pkg/linux/build-napi-inside-docker.sh + - name: Test NAPI file in other containers +if: matrix.image == 'linux_glibc' +run: | + ./tests/load-test.sh node:16-buster Review Comment: These tests are added to verify `Pulsar.node` can be loaded by Node.js with higher version libstdc++.so dependency. -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar-client-node] BewareMyPower commented on a diff in pull request #288: Make it compatible with GLIBCXX_3.4.19 (CentOS 7)
BewareMyPower commented on code in PR #288: URL: https://github.com/apache/pulsar-client-node/pull/288#discussion_r1073646699 ## tests/docker-load-test.sh: ## @@ -0,0 +1,35 @@ +#!/bin/bash +# +# 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. +# + +# NOTE: This script should only run inside a Node.js docker container. +# See ./load-test.sh for details. +set -ex + +# Create an empty directory to test +mkdir -p /app && cd /app +tar zxf /pulsar-client-node/pulsar-client-node.tar.gz +cp /pulsar-client-node/build/Release/Pulsar.node . Review Comment: Good. I have verified it. -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #15576: [PIP-167][Authorization] Make it Configurable to Require Subscription Permission
michaeljmarshall commented on PR #15576: URL: https://github.com/apache/pulsar/pull/15576#issuecomment-1387204901 @momo-jun this PR was reverted, so no docs are needed. -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar-client-node] BewareMyPower commented on a diff in pull request #288: Make it compatible with GLIBCXX_3.4.19 (CentOS 7)
BewareMyPower commented on code in PR #288: URL: https://github.com/apache/pulsar-client-node/pull/288#discussion_r1073643521 ## tests/load-test.sh: ## @@ -0,0 +1,42 @@ +#!/bin/bash Review Comment: IMO, It's good to: - Put `load_test.js` under `tests/linux` - Rename `load_test.js` with `linux_load_test.js` It's bad to put `load_test.js` under `pkg/linux`. -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar-client-node] BewareMyPower commented on a diff in pull request #288: Make it compatible with GLIBCXX_3.4.19 (CentOS 7)
BewareMyPower commented on code in PR #288: URL: https://github.com/apache/pulsar-client-node/pull/288#discussion_r1073636981 ## .github/workflows/ci-pr-validation.yml: ## @@ -167,6 +167,14 @@ jobs: docker run -i -v $PWD:/pulsar-client-node build:latest \ /pulsar-client-node/pkg/linux/build-napi-inside-docker.sh + - name: Test NAPI file in other containers +if: matrix.image == 'linux_glibc' +run: | + ./tests/load-test.sh node:16-buster Review Comment: Just as the GLIBCXX compatibility issue here https://github.com/apache/pulsar-client-node/pull/288#discussion_r1073622557, installing Node.js 18 on some systems could be very complicated. The only way is to compiling it from source. Since NAPI guarantees the ABI compatibility, why do we insist on verifying with Node.js 18? -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar-client-node] BewareMyPower commented on a diff in pull request #288: Make it compatible with GLIBCXX_3.4.19 (CentOS 7)
BewareMyPower commented on code in PR #288: URL: https://github.com/apache/pulsar-client-node/pull/288#discussion_r1073627622 ## .github/workflows/ci-pr-validation.yml: ## @@ -167,6 +167,14 @@ jobs: docker run -i -v $PWD:/pulsar-client-node build:latest \ /pulsar-client-node/pkg/linux/build-napi-inside-docker.sh + - name: Test NAPI file in other containers +if: matrix.image == 'linux_glibc' +run: | + ./tests/load-test.sh node:16-buster Review Comment: The `node:-` image just represents a specific node version and OS. For example, `node:16-buster` represents Node.js 16 and Debian-buster. CentOS 7 has already been verified when building the `Pulsar.node`. There are many Linux distributions and they cannot be covered all. And it will require some extra efforts to install Node.js on them. -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar-client-node] BewareMyPower commented on a diff in pull request #288: Make it compatible with GLIBCXX_3.4.19 (CentOS 7)
BewareMyPower commented on code in PR #288:
URL:
https://github.com/apache/pulsar-client-node/pull/288#discussion_r1073622557
##
pkg/linux/Dockerfile_linux_glibc:
##
@@ -17,15 +17,19 @@
# under the License.
#
-ARG NODE_VERSION
+FROM centos:7
-FROM node:${NODE_VERSION}-buster
+RUN yum update -y
+RUN yum install -y gcc gcc-c++ make python3
-RUN apt-get update -y && \
- apt-get install -y \
-curl \
-g++ \
-make \
-python3
+ARG ARCH
+WORKDIR /app
+RUN SUFFIX=$(echo ${ARCH} | sed 's/86_//') && \
+ echo $SUFFIX && \
+ curl -O -L
https://nodejs.org/download/release/v16.19.0/node-v16.19.0-linux-$SUFFIX.tar.gz
&& \
Review Comment:
> It has nothing to do with the Node.js version, right?
The pre-built Node.js binary is built with a specific C++ compiler. So there
is a GLIBCXX requirement for the `node` binary itself.
You can run the `centos:7` container first:
```bash
docker run --rm -it centos:7 /bin/bash
```
Then run:
```bash
yum update -y
curl -O -L https://nodejs.org/dist/v18.13.0/node-v18.13.0-linux-x64.tar.xz
tar xf node-v18.13.0-linux-x64.tar.xz
./node-v18.13.0-linux-x64/bin/node
```
You will see the following error:
```bash
./node-v18.13.0-linux-x64/bin/node: /lib64/libm.so.6: version `GLIBC_2.27'
not found (required by ./node-v18.13.0-linux-x64/bin/node)
./node-v18.13.0-linux-x64/bin/node: /lib64/libc.so.6: version `GLIBC_2.25'
not found (required by ./node-v18.13.0-linux-x64/bin/node)
./node-v18.13.0-linux-x64/bin/node: /lib64/libc.so.6: version `GLIBC_2.28'
not found (required by ./node-v18.13.0-linux-x64/bin/node)
./node-v18.13.0-linux-x64/bin/node: /lib64/libstdc++.so.6: version
`CXXABI_1.3.9' not found (required by ./node-v18.13.0-linux-x64/bin/node)
./node-v18.13.0-linux-x64/bin/node: /lib64/libstdc++.so.6: version
`GLIBCXX_3.4.20' not found (required by ./node-v18.13.0-linux-x64/bin/node)
./node-v18.13.0-linux-x64/bin/node: /lib64/libstdc++.so.6: version
`GLIBCXX_3.4.21' not found (required by ./node-v18.13.0-linux-x64/bin/node)
```
Though maybe we can use other ways to install Node.js 18 on CentOS 7, but it
could be much more complicated.
We only need to build the `Pulsar.node` with the NAPI of Node.js and the
compatibility of NAPI is good. So using Node.js 16 on CentOS 7 is not a problem.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] liangyepianzhou commented on pull request #19273: [fix][txn] Set TC client ready after all connections ready
liangyepianzhou commented on PR #19273: URL: https://github.com/apache/pulsar/pull/19273#issuecomment-1387171783 > I have two questions/points: > > 1. what happens if some of the coordinators are not available ? > 2. is this change adding more latency in the creation of the PulsarClient ? The TC client is created synchronized, so this change will not add any latency. If the coordinators are unavailable, the client will try to connect later. https://github.com/apache/pulsar/blob/ea4f7eba335e72e1d298ec127bc0d140be2b7be2/pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java#L218-L226 -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] nodece commented on a diff in pull request #19256: [fix][websocket] Fix webSocketPingDurationSeconds config
nodece commented on code in PR #19256:
URL: https://github.com/apache/pulsar/pull/19256#discussion_r1073613961
##
pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConfiguration.java:
##
@@ -803,6 +803,12 @@ public class ProxyConfiguration implements
PulsarConfiguration {
)
private boolean webSocketServiceEnabled = false;
+@FieldContext(
+category = CATEGORY_WEBSOCKET,
+doc = "Interval of time to sending the ping to keep alive in
WebSocket proxy. "
++ "This value greater than 0 means enabled")
+private int webSocketPingDurationSeconds = -1;
Review Comment:
We already add this to other branches, so I don't suggest rename to
`keepAliveIntervalSeconds`.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar-client-node] BewareMyPower commented on a diff in pull request #288: Make it compatible with GLIBCXX_3.4.19 (CentOS 7)
BewareMyPower commented on code in PR #288: URL: https://github.com/apache/pulsar-client-node/pull/288#discussion_r1073613508 ## tests/load-test.sh: ## @@ -0,0 +1,42 @@ +#!/bin/bash Review Comment: But `pkg/linux` directory should represent packaging the Node.js client on Linux. The script here is to test `load_test.js` on Linux systems. Assuming there are 3 scripts (e.g. `load-test-win.sh`, `load-test-osx.sh`, `load-test-linux.sh`) that test `load_test.js` on Windows, macOS and Linux. Do you think it's better to put these scripts under three different directories like `pkg/windows`, `pkg/linux/` and `pkg/macos`? -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar-client-node] BewareMyPower commented on a diff in pull request #288: Make it compatible with GLIBCXX_3.4.19 (CentOS 7)
BewareMyPower commented on code in PR #288: URL: https://github.com/apache/pulsar-client-node/pull/288#discussion_r1073610275 ## tests/docker-load-test.sh: ## @@ -0,0 +1,35 @@ +#!/bin/bash +# +# 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. +# + +# NOTE: This script should only run inside a Node.js docker container. +# See ./load-test.sh for details. +set -ex + +# Create an empty directory to test +mkdir -p /app && cd /app +tar zxf /pulsar-client-node/pulsar-client-node.tar.gz +cp /pulsar-client-node/build/Release/Pulsar.node . Review Comment: > When /pulsar-client-node/lib/binding/Pulsar.node, Did you mean when `/pulsar-client-node/lib/binding/Pulsar.node` already exists? -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] andrasbeni commented on a diff in pull request #19208: [improve][broker]Enable custom metadata stores
andrasbeni commented on code in PR #19208:
URL: https://github.com/apache/pulsar/pull/19208#discussion_r1073578096
##
pulsar-metadata/src/test/java/org/apache/pulsar/metadata/impl/MetadataStoreFactoryImplTest.java:
##
@@ -0,0 +1,125 @@
+/*
+ * 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.metadata.impl;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertTrue;
+import org.apache.pulsar.metadata.api.GetResult;
+import org.apache.pulsar.metadata.api.MetadataStore;
+import org.apache.pulsar.metadata.api.MetadataStoreConfig;
+import org.apache.pulsar.metadata.api.MetadataStoreException;
+import org.apache.pulsar.metadata.api.MetadataStoreProvider;
+import org.apache.pulsar.metadata.api.Stat;
+import org.apache.pulsar.metadata.api.extended.CreateOption;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+import java.util.EnumSet;
+import java.util.List;
+import java.util.Optional;
+import java.util.concurrent.CompletableFuture;
+
+public class MetadataStoreFactoryImplTest {
+
+private static Object originalProperty;
+
+@BeforeClass
+public void setMetadataStoreProperty() {
+originalProperty =
System.getProperties().get(MetadataStoreFactoryImpl.METADATASTORE_PROVIDERS_PROPERTY);
Review Comment:
It certainly does. But I think it is a good practice to leave the static
state as it was before the test, regardless of the probability of it being
non-null.
On the other hand, I think it's possible for someone using this very feature
to run tests with their own provider configured in system properties.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar-client-cpp] BewareMyPower commented on a diff in pull request #157: [feat] Support auto download schema when create producer.
BewareMyPower commented on code in PR #157:
URL: https://github.com/apache/pulsar-client-cpp/pull/157#discussion_r1073576266
##
tests/ClientTest.cc:
##
@@ -305,6 +305,7 @@ TEST(ClientTest, testCloseClient) {
auto t0 = std::chrono::steady_clock::now();
while ((std::chrono::steady_clock::now() - t0) <
std::chrono::microseconds(i)) {
}
+sleep(1);
Review Comment:
Please move it into another PR if you want to fix the flaky test. BTW,
sleeping 1 second is unacceptable here because it's in a 1000 times loop
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar-client-cpp] BewareMyPower commented on a diff in pull request #157: [feat] Support auto download schema when create producer.
BewareMyPower commented on code in PR #157:
URL: https://github.com/apache/pulsar-client-cpp/pull/157#discussion_r1073574213
##
lib/HTTPLookupService.cc:
##
@@ -409,4 +430,72 @@ void
HTTPLookupService::handleLookupHTTPRequest(LookupPromise promise, const std
}
}
+void HTTPLookupService::handleGetSchemaHTTPRequest(GetSchemaPromise promise,
const std::string completeUrl) {
+std::string responseData;
+Result result = sendHTTPRequest(completeUrl, responseData);
+
+if (result == ResultNotFound) {
+promise.setValue(boost::none);
Review Comment:
If the `ResultNotFound` is only used internally, it should not be exposed to
users. `ResultRetryable`, which was added by me, is a bad example but I cannot
think of a better way because of the deeply coupling code. IMO, it would be
acceptable to remove `ResultRetryable` from the enum if there was a better way
to achieve the same goal.
`sendHTTPRequest` is a private method of `HTTPLookupService`, we can return
the `long` response code instead of the `Result`. Then, we can map the response
code to a `Result` when the code is not 404.
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar-client-cpp] BewareMyPower commented on a diff in pull request #157: [feat] Support auto download schema when create producer.
BewareMyPower commented on code in PR #157: URL: https://github.com/apache/pulsar-client-cpp/pull/157#discussion_r1073564343 ## include/pulsar/Result.h: ## @@ -94,6 +94,7 @@ enum Result ResultInterrupted, /// Interrupted while waiting to dequeue ResultDisconnected, /// Client connection has been disconnected +ResultNotFound /// The generic was not found Review Comment: Could this `Result` be returned with binary protocol? If it's an error that is only related to the HTTP protocol, I think it would not be a good idea to expose it as a `Result`. -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] Dishwasha commented on pull request #8527: [feat][io] Add support for partitioned tables
Dishwasha commented on PR #8527: URL: https://github.com/apache/pulsar/pull/8527#issuecomment-1387090279 > Hi @Dishwasha, do you have any updates for adding the docs? Since 2.11 has been released, now it's good timing to start the doc update in the next version. I am not an active maintainer of this project and made this extremely small contribution over 2 years ago. -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] liangyepianzhou opened a new pull request, #19273: [fix][txn] Set Tc client ready after all connections ready
liangyepianzhou opened a new pull request, #19273: URL: https://github.com/apache/pulsar/pull/19273 ### Motivation logic flow in `startAsync` 1. change the state of the TCClient to `STARTING` 2. Connecting to the transaction coordinators 3. Set the state to `READY` 4. Connect fails but the state is READY ### Modification Set the state to ready after all connections are complete. ### 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` - [x] `doc-not-needed` - [ ] `doc-complete` ### Matching PR in forked repository PR in forked repository: -- 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. To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a diff in pull request #19256: [fix][websocket] Fix webSocketPingDurationSeconds config
codelipenghui commented on code in PR #19256:
URL: https://github.com/apache/pulsar/pull/19256#discussion_r1073509207
##
pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConfiguration.java:
##
@@ -803,6 +803,12 @@ public class ProxyConfiguration implements
PulsarConfiguration {
)
private boolean webSocketServiceEnabled = false;
+@FieldContext(
+category = CATEGORY_WEBSOCKET,
+doc = "Interval of time to sending the ping to keep alive in
WebSocket proxy. "
++ "This value greater than 0 means enabled")
+private int webSocketPingDurationSeconds = -1;
Review Comment:
It is better to use `webSocketKeepAliveIntervalSeconds` to make it
consistent with the existing name `keepAliveIntervalSeconds`
And we should also add `keepAliveIntervalSeconds` to `websocket.conf`
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
