From 0e8e50ae2609e9f681bb7fb9f7d24cf118b1e6e7 Mon Sep 17 00:00:00 2001 From: Matteo Merli Date: Thu, 13 Feb 2025 07:56:13 -0800 Subject: [PATCH 01/16] [fix] Potential fix for code scanning alert no. 22: HTTP response splitting (#23976) Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- .../authentication/AuthenticationProviderSasl.java | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/pulsar-broker-auth-sasl/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderSasl.java b/pulsar-broker-auth-sasl/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderSasl.java index f8841193ba2d2..351f8d9cfd364 100644 --- a/pulsar-broker-auth-sasl/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderSasl.java +++ b/pulsar-broker-auth-sasl/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderSasl.java @@ -271,7 +271,7 @@ public boolean authenticateHttpRequest(HttpServletRequest request, HttpServletRe } else { checkState(request.getHeader(SASL_HEADER_STATE).equalsIgnoreCase(SASL_STATE_SERVER_CHECK_TOKEN)); setResponseHeaderState(response, SASL_STATE_COMPLETE); - response.setHeader(SASL_STATE_SERVER, request.getHeader(SASL_STATE_SERVER)); + response.setHeader(SASL_STATE_SERVER, sanitizeHeaderValue(request.getHeader(SASL_STATE_SERVER))); response.setStatus(HttpServletResponse.SC_OK); if (log.isDebugEnabled()) { log.debug("[{}] Server side role token verified success: {}", request.getRequestURI(), @@ -325,4 +325,12 @@ public boolean authenticateHttpRequest(HttpServletRequest request, HttpServletRe } } } + + private String sanitizeHeaderValue(String value) { + if (value == null) { + return null; + } + // Remove CRLF and other special characters + return value.replaceAll("[\\r\\n]", "").replaceAll("[^\\x20-\\x7E]", ""); + } } From d3ea0ee8515949808f2067c3cc2874ac379b5f28 Mon Sep 17 00:00:00 2001 From: Zixuan Liu Date: Fri, 14 Feb 2025 01:50:53 +0800 Subject: [PATCH 02/16] [fix][io] Fix pulsar-io:pom not found (#23979) Signed-off-by: Zixuan Liu --- pulsar-io/aerospike/pom.xml | 7 +++++++ pulsar-io/alluxio/pom.xml | 7 +++++++ pulsar-io/aws/pom.xml | 7 +++++++ pulsar-io/batch-data-generator/pom.xml | 7 +++++++ pulsar-io/canal/pom.xml | 7 +++++++ pulsar-io/cassandra/pom.xml | 7 +++++++ pulsar-io/common/pom.xml | 7 ------- pulsar-io/core/pom.xml | 7 ------- pulsar-io/data-generator/pom.xml | 7 +++++++ pulsar-io/debezium/core/pom.xml | 12 ++++++++++++ pulsar-io/debezium/mongodb/pom.xml | 7 +++++++ pulsar-io/debezium/mssql/pom.xml | 7 +++++++ pulsar-io/debezium/mysql/pom.xml | 7 +++++++ pulsar-io/debezium/oracle/pom.xml | 7 +++++++ pulsar-io/debezium/postgres/pom.xml | 7 +++++++ pulsar-io/docs/pom.xml | 7 +++++++ pulsar-io/dynamodb/pom.xml | 7 +++++++ pulsar-io/elastic-search/pom.xml | 7 +++++++ pulsar-io/file/pom.xml | 7 +++++++ pulsar-io/flume/pom.xml | 7 +++++++ pulsar-io/hbase/pom.xml | 7 +++++++ pulsar-io/hdfs3/pom.xml | 7 +++++++ pulsar-io/http/pom.xml | 7 +++++++ pulsar-io/influxdb/pom.xml | 7 +++++++ pulsar-io/jdbc/clickhouse/pom.xml | 7 +++++++ pulsar-io/jdbc/core/pom.xml | 12 ++++++++++++ pulsar-io/jdbc/mariadb/pom.xml | 7 +++++++ pulsar-io/jdbc/openmldb/pom.xml | 7 +++++++ pulsar-io/jdbc/postgres/pom.xml | 7 +++++++ pulsar-io/jdbc/sqlite/pom.xml | 7 +++++++ pulsar-io/kafka-connect-adaptor-nar/pom.xml | 7 +++++++ pulsar-io/kafka-connect-adaptor/pom.xml | 11 +++++++++++ pulsar-io/kafka/pom.xml | 7 +++++++ pulsar-io/kinesis/pom.xml | 7 +++++++ pulsar-io/mongo/pom.xml | 7 +++++++ pulsar-io/netty/pom.xml | 7 +++++++ pulsar-io/nsq/pom.xml | 7 +++++++ pulsar-io/pom.xml | 7 ------- pulsar-io/rabbitmq/pom.xml | 7 +++++++ pulsar-io/redis/pom.xml | 7 +++++++ pulsar-io/solr/pom.xml | 7 +++++++ pulsar-io/twitter/pom.xml | 7 +++++++ 42 files changed, 287 insertions(+), 21 deletions(-) diff --git a/pulsar-io/aerospike/pom.xml b/pulsar-io/aerospike/pom.xml index 7ae3ee0c99ffd..031f4a2bdf5b2 100644 --- a/pulsar-io/aerospike/pom.xml +++ b/pulsar-io/aerospike/pom.xml @@ -68,6 +68,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/alluxio/pom.xml b/pulsar-io/alluxio/pom.xml index a89299a3a74a3..38ff0fbb1bad9 100644 --- a/pulsar-io/alluxio/pom.xml +++ b/pulsar-io/alluxio/pom.xml @@ -119,6 +119,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/aws/pom.xml b/pulsar-io/aws/pom.xml index ed2d281b49741..d71e436d4557c 100644 --- a/pulsar-io/aws/pom.xml +++ b/pulsar-io/aws/pom.xml @@ -59,6 +59,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + com.github.spotbugs spotbugs-maven-plugin diff --git a/pulsar-io/batch-data-generator/pom.xml b/pulsar-io/batch-data-generator/pom.xml index fcbc0823b558d..3f36a2efc4d51 100644 --- a/pulsar-io/batch-data-generator/pom.xml +++ b/pulsar-io/batch-data-generator/pom.xml @@ -73,6 +73,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/canal/pom.xml b/pulsar-io/canal/pom.xml index 8d3578ea2b054..9a7d8965ee425 100644 --- a/pulsar-io/canal/pom.xml +++ b/pulsar-io/canal/pom.xml @@ -123,6 +123,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/cassandra/pom.xml b/pulsar-io/cassandra/pom.xml index 7cd51431183e7..d38ed2542d117 100644 --- a/pulsar-io/cassandra/pom.xml +++ b/pulsar-io/cassandra/pom.xml @@ -57,6 +57,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/common/pom.xml b/pulsar-io/common/pom.xml index 7eef536b2b6b6..987c6c8cc8b75 100644 --- a/pulsar-io/common/pom.xml +++ b/pulsar-io/common/pom.xml @@ -48,13 +48,6 @@ - - org.apache.maven.plugins - maven-deploy-plugin - - false - - com.github.spotbugs spotbugs-maven-plugin diff --git a/pulsar-io/core/pom.xml b/pulsar-io/core/pom.xml index fa4236bf870fe..8f7920c7255ed 100644 --- a/pulsar-io/core/pom.xml +++ b/pulsar-io/core/pom.xml @@ -40,13 +40,6 @@ - - org.apache.maven.plugins - maven-deploy-plugin - - false - - com.github.spotbugs spotbugs-maven-plugin diff --git a/pulsar-io/data-generator/pom.xml b/pulsar-io/data-generator/pom.xml index bf21af7c0c6cc..9cc941f8b24e7 100644 --- a/pulsar-io/data-generator/pom.xml +++ b/pulsar-io/data-generator/pom.xml @@ -60,6 +60,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/debezium/core/pom.xml b/pulsar-io/debezium/core/pom.xml index 297ac64dd89d6..cd7ac90263dd7 100644 --- a/pulsar-io/debezium/core/pom.xml +++ b/pulsar-io/debezium/core/pom.xml @@ -116,4 +116,16 @@ + + + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + + + + diff --git a/pulsar-io/debezium/mongodb/pom.xml b/pulsar-io/debezium/mongodb/pom.xml index 723f79cbef0c8..b4c1423498000 100644 --- a/pulsar-io/debezium/mongodb/pom.xml +++ b/pulsar-io/debezium/mongodb/pom.xml @@ -55,6 +55,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/debezium/mssql/pom.xml b/pulsar-io/debezium/mssql/pom.xml index 5721ab90b6e79..5ee0ffaedb505 100644 --- a/pulsar-io/debezium/mssql/pom.xml +++ b/pulsar-io/debezium/mssql/pom.xml @@ -55,6 +55,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/debezium/mysql/pom.xml b/pulsar-io/debezium/mysql/pom.xml index 0896d079f35d7..8a168f621e2f2 100644 --- a/pulsar-io/debezium/mysql/pom.xml +++ b/pulsar-io/debezium/mysql/pom.xml @@ -65,6 +65,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/debezium/oracle/pom.xml b/pulsar-io/debezium/oracle/pom.xml index 8af034729f79e..1f62811b3a9eb 100644 --- a/pulsar-io/debezium/oracle/pom.xml +++ b/pulsar-io/debezium/oracle/pom.xml @@ -55,6 +55,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/debezium/postgres/pom.xml b/pulsar-io/debezium/postgres/pom.xml index 7d6cae61ab74f..92b9870d5f917 100644 --- a/pulsar-io/debezium/postgres/pom.xml +++ b/pulsar-io/debezium/postgres/pom.xml @@ -62,6 +62,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/docs/pom.xml b/pulsar-io/docs/pom.xml index 1bfc70844eb76..5f04f0e500151 100644 --- a/pulsar-io/docs/pom.xml +++ b/pulsar-io/docs/pom.xml @@ -221,6 +221,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + com.github.spotbugs spotbugs-maven-plugin diff --git a/pulsar-io/dynamodb/pom.xml b/pulsar-io/dynamodb/pom.xml index c08ab329dbb93..6b1ec663e08dd 100644 --- a/pulsar-io/dynamodb/pom.xml +++ b/pulsar-io/dynamodb/pom.xml @@ -101,6 +101,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/elastic-search/pom.xml b/pulsar-io/elastic-search/pom.xml index 674f40f3f4a78..53093b91ee75a 100644 --- a/pulsar-io/elastic-search/pom.xml +++ b/pulsar-io/elastic-search/pom.xml @@ -109,6 +109,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/file/pom.xml b/pulsar-io/file/pom.xml index 055469a4958f4..d784d65d516b6 100644 --- a/pulsar-io/file/pom.xml +++ b/pulsar-io/file/pom.xml @@ -60,6 +60,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/flume/pom.xml b/pulsar-io/flume/pom.xml index ff77be777f98d..11dc04e9a98d7 100644 --- a/pulsar-io/flume/pom.xml +++ b/pulsar-io/flume/pom.xml @@ -122,6 +122,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/hbase/pom.xml b/pulsar-io/hbase/pom.xml index 2777524d2f311..32ca117da91d0 100644 --- a/pulsar-io/hbase/pom.xml +++ b/pulsar-io/hbase/pom.xml @@ -89,6 +89,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/hdfs3/pom.xml b/pulsar-io/hdfs3/pom.xml index d20a2ef208e27..3e6bc2b45cc36 100644 --- a/pulsar-io/hdfs3/pom.xml +++ b/pulsar-io/hdfs3/pom.xml @@ -91,6 +91,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/http/pom.xml b/pulsar-io/http/pom.xml index 41af3be06e0e6..652d6d123b394 100644 --- a/pulsar-io/http/pom.xml +++ b/pulsar-io/http/pom.xml @@ -77,6 +77,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/influxdb/pom.xml b/pulsar-io/influxdb/pom.xml index 4b21005a20cd6..0ef2876b8629c 100644 --- a/pulsar-io/influxdb/pom.xml +++ b/pulsar-io/influxdb/pom.xml @@ -83,6 +83,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/jdbc/clickhouse/pom.xml b/pulsar-io/jdbc/clickhouse/pom.xml index fedafc8569578..e60e311de3cef 100644 --- a/pulsar-io/jdbc/clickhouse/pom.xml +++ b/pulsar-io/jdbc/clickhouse/pom.xml @@ -54,6 +54,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/jdbc/core/pom.xml b/pulsar-io/jdbc/core/pom.xml index 8f9d2b071c7a4..14bbc2d4bcc83 100644 --- a/pulsar-io/jdbc/core/pom.xml +++ b/pulsar-io/jdbc/core/pom.xml @@ -79,4 +79,16 @@ + + + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + + + + \ No newline at end of file diff --git a/pulsar-io/jdbc/mariadb/pom.xml b/pulsar-io/jdbc/mariadb/pom.xml index 4eb0370f92b48..1f9094f34c1d8 100644 --- a/pulsar-io/jdbc/mariadb/pom.xml +++ b/pulsar-io/jdbc/mariadb/pom.xml @@ -46,6 +46,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/jdbc/openmldb/pom.xml b/pulsar-io/jdbc/openmldb/pom.xml index 22e4bbcc4685f..91a74f5fdd2cd 100644 --- a/pulsar-io/jdbc/openmldb/pom.xml +++ b/pulsar-io/jdbc/openmldb/pom.xml @@ -63,6 +63,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/jdbc/postgres/pom.xml b/pulsar-io/jdbc/postgres/pom.xml index 83afb21a37187..310920e2a9b6d 100644 --- a/pulsar-io/jdbc/postgres/pom.xml +++ b/pulsar-io/jdbc/postgres/pom.xml @@ -47,6 +47,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/jdbc/sqlite/pom.xml b/pulsar-io/jdbc/sqlite/pom.xml index 5c600acf0b398..e5eaec2e9382a 100644 --- a/pulsar-io/jdbc/sqlite/pom.xml +++ b/pulsar-io/jdbc/sqlite/pom.xml @@ -63,6 +63,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/kafka-connect-adaptor-nar/pom.xml b/pulsar-io/kafka-connect-adaptor-nar/pom.xml index 22684a4f0e8b8..29af900ad0e57 100644 --- a/pulsar-io/kafka-connect-adaptor-nar/pom.xml +++ b/pulsar-io/kafka-connect-adaptor-nar/pom.xml @@ -41,6 +41,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/kafka-connect-adaptor/pom.xml b/pulsar-io/kafka-connect-adaptor/pom.xml index a33ce25371a41..b12665eb4785a 100644 --- a/pulsar-io/kafka-connect-adaptor/pom.xml +++ b/pulsar-io/kafka-connect-adaptor/pom.xml @@ -214,4 +214,15 @@ + + + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + + + diff --git a/pulsar-io/kafka/pom.xml b/pulsar-io/kafka/pom.xml index ca64c9c194290..9ed110d4f2941 100644 --- a/pulsar-io/kafka/pom.xml +++ b/pulsar-io/kafka/pom.xml @@ -130,6 +130,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/kinesis/pom.xml b/pulsar-io/kinesis/pom.xml index 64edc9670a774..e2aabb3f3ca99 100644 --- a/pulsar-io/kinesis/pom.xml +++ b/pulsar-io/kinesis/pom.xml @@ -149,6 +149,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/mongo/pom.xml b/pulsar-io/mongo/pom.xml index 0cc3668ede5c1..b065fe1b123ba 100644 --- a/pulsar-io/mongo/pom.xml +++ b/pulsar-io/mongo/pom.xml @@ -72,6 +72,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/netty/pom.xml b/pulsar-io/netty/pom.xml index 3706df72e0677..0d6759a58045d 100644 --- a/pulsar-io/netty/pom.xml +++ b/pulsar-io/netty/pom.xml @@ -72,6 +72,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/nsq/pom.xml b/pulsar-io/nsq/pom.xml index c882e7b9b42d0..5384948bec0f6 100644 --- a/pulsar-io/nsq/pom.xml +++ b/pulsar-io/nsq/pom.xml @@ -68,6 +68,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/pom.xml b/pulsar-io/pom.xml index 968eb81b24614..4eba7553883f1 100644 --- a/pulsar-io/pom.xml +++ b/pulsar-io/pom.xml @@ -148,13 +148,6 @@ - - org.apache.maven.plugins - maven-deploy-plugin - - ${skipDeployConnector} - - org.apache.maven.plugins maven-checkstyle-plugin diff --git a/pulsar-io/rabbitmq/pom.xml b/pulsar-io/rabbitmq/pom.xml index a82b37ca968a2..42fbf2d1ced77 100644 --- a/pulsar-io/rabbitmq/pom.xml +++ b/pulsar-io/rabbitmq/pom.xml @@ -110,6 +110,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/redis/pom.xml b/pulsar-io/redis/pom.xml index 0de19ce3d67a1..edb2ec19d3884 100644 --- a/pulsar-io/redis/pom.xml +++ b/pulsar-io/redis/pom.xml @@ -88,6 +88,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/solr/pom.xml b/pulsar-io/solr/pom.xml index 88e09823a5b44..4f83196aca950 100644 --- a/pulsar-io/solr/pom.xml +++ b/pulsar-io/solr/pom.xml @@ -112,6 +112,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin diff --git a/pulsar-io/twitter/pom.xml b/pulsar-io/twitter/pom.xml index e909dc16d4f43..c4b38dbb10618 100644 --- a/pulsar-io/twitter/pom.xml +++ b/pulsar-io/twitter/pom.xml @@ -74,6 +74,13 @@ + + org.apache.maven.plugins + maven-deploy-plugin + + ${skipDeployConnector} + + org.apache.nifi nifi-nar-maven-plugin From 6a7284cb894f0db603fda9b4b949bc004c963df9 Mon Sep 17 00:00:00 2001 From: Rajan Dhabalia Date: Thu, 13 Feb 2025 16:09:38 -0800 Subject: [PATCH 03/16] [improve] [pip] PIP-395: Add Proxy configuration to support configurable response headers for http reverse-proxy (#23648) --- pip/pip-395.md | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 81 insertions(+) create mode 100644 pip/pip-395.md diff --git a/pip/pip-395.md b/pip/pip-395.md new file mode 100644 index 0000000000000..9c5c15c83ed52 --- /dev/null +++ b/pip/pip-395.md @@ -0,0 +1,81 @@ +# PIP-395: Add Proxy configuration to support configurable response headers for http reverse-proxy + + +# Motivation + +Pulsar Proxy has a support to use it as HTTP reverse proxy to access Broker’s admin API and can also be extended to act as a reverse proxy other HTTP endpoints. Sometimes, it is very crucial to add customizable headers into the HTTP response returned by Proxy to enhance the security experience while using Proxy over HTTP. + +Response headers in a reverse HTTP proxy are critical for maintaining and enhancing the security of the web applications and services behind the proxy. These headers act as a first line of defense, hardening the web server infrastructure and protecting clients from common web vulnerabilities. + +For example, when implementing a reverse HTTP proxy, security headers such as `Referrer-Policy`, `X-Content-Type-Options`, `Strict-Transport-Security`, `X-Content-Type-Options`, etc., are useful to prevent security attacks like clickjacking, MIME-sniffing, data leakage, and more. So, such headers play a crucial role in enhancing the security posture of proxy infrastructure. + +Therefore, we would like to add support into Pulsar Proxy where users can add custom response headers by passing them into the configuration. This PIP will add this support by adding a new configuration called `proxyHttpResponseHeadersJson` where user can pass multiple headers with key-value map into the json format. Proxy server will retrieve headers from this configuration and pass it as response headers for every http request when user wants to use Pulsar Proxy as an HTTP reverse proxy. + + +# Goals + +## In Scope + +Add a new configuration `proxyHttpResponseHeadersJson` to the Proxy configuration. +eg: +``` +proxyHttpResponseHeadersJson=`{"header1":"value1","header2":"value2"}` +``` + +## Out of Scope + +# High Level Design + +# Detailed Design + +## Design & Implementation Details + +Add a new configuration `proxyHttpResponseHeadersJson` to the Proxy configuration. +This configuration will allow the user to set default headers which proxy will return into the response headers for every http request which proxy will receive as a reverse proxy. + + +### Public API +NA +### Binary protocol + +### Configuration + +### CLI + +### Metrics + +NA + +# Monitoring + +NA + +# Security Considerations + +NA + +# Backward & Forward Compatibility + +## Upgrade + +This is a new feature, and it does not affect the existing configuration. + +## Downgrade / Rollback + +Rollback will not impact the existing functionality. + +## Pulsar Geo-Replication Upgrade & Downgrade/Rollback Considerations + + + +# Alternatives + + + +# General Notes + +# Links \ No newline at end of file From 6949ad091b77a6e06d20b695b8d1673a7d310272 Mon Sep 17 00:00:00 2001 From: Rajan Dhabalia Date: Thu, 13 Feb 2025 16:10:14 -0800 Subject: [PATCH 04/16] [improve] [proxy] PIP-395: Add Proxy configuration to support configurable response headers for http reverse-proxy (#23649) --- conf/proxy.conf | 4 ++ .../proxy/server/ProxyConfiguration.java | 7 +++ .../apache/pulsar/proxy/server/WebServer.java | 51 +++++++++++++++++++ .../server/ProxyAdditionalServletTest.java | 16 +++++- 4 files changed, 76 insertions(+), 2 deletions(-) diff --git a/conf/proxy.conf b/conf/proxy.conf index 6e6c960e8009e..567cc0772a310 100644 --- a/conf/proxy.conf +++ b/conf/proxy.conf @@ -63,6 +63,10 @@ advertisedAddress= # If true, the real IP addresses of consumers and producers can be obtained when getting topic statistics data. haProxyProtocolEnabled=false +# Default http header map to add into http-proxy for the any security requirements. +# eg: {"header1":"value"} +proxyHttpResponseHeadersJson= + # Enable or disable the use of HA proxy protocol for resolving the client IP for http/https requests. webServiceHaProxyProtocolEnabled=false diff --git a/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConfiguration.java b/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConfiguration.java index b9360e403f6f4..329e6d52ba6f1 100644 --- a/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConfiguration.java +++ b/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConfiguration.java @@ -818,6 +818,13 @@ public class ProxyConfiguration implements PulsarConfiguration { ) private Set proxyAdditionalServlets = new TreeSet<>(); + @FieldContext( + category = CATEGORY_PLUGIN, + doc = "Default http header map to add into http-proxy for the any security requirements " + + "eg: { \"header1\": \"val1\", \"header2\": \"val2\" }" + ) + private String proxyHttpResponseHeadersJson; + @FieldContext( category = CATEGORY_PLUGIN, doc = "List of proxy additional servlet to load, which is a list of proxy additional servlet names" diff --git a/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/WebServer.java b/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/WebServer.java index 3c472135bdfb0..7591b8b54db49 100644 --- a/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/WebServer.java +++ b/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/WebServer.java @@ -19,6 +19,7 @@ package org.apache.pulsar.proxy.server; import static org.apache.pulsar.proxy.server.AdminProxyHandler.INIT_PARAM_REQUEST_BUFFER_SIZE; +import com.fasterxml.jackson.core.JsonProcessingException; import io.opentelemetry.api.OpenTelemetry; import io.prometheus.client.jetty.JettyStatisticsCollector; import java.io.IOException; @@ -27,12 +28,22 @@ import java.util.Arrays; import java.util.Collections; import java.util.EnumSet; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.Optional; import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; import javax.servlet.DispatcherType; +import javax.servlet.Filter; +import javax.servlet.FilterChain; +import javax.servlet.FilterConfig; +import javax.servlet.ServletException; +import javax.servlet.ServletRequest; +import javax.servlet.ServletResponse; +import javax.servlet.http.HttpServletResponse; +import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.tuple.Pair; import org.apache.pulsar.broker.authentication.AuthenticationService; import org.apache.pulsar.broker.web.AuthenticationFilter; @@ -41,6 +52,7 @@ import org.apache.pulsar.broker.web.RateLimitingFilter; import org.apache.pulsar.broker.web.WebExecutorThreadPool; import org.apache.pulsar.client.util.ExecutorProvider; +import org.apache.pulsar.common.util.ObjectMapperFactory; import org.apache.pulsar.common.util.PulsarSslConfiguration; import org.apache.pulsar.common.util.PulsarSslFactory; import org.apache.pulsar.jetty.tls.JettySslContextFactory; @@ -242,6 +254,7 @@ private void addServlet(String basePath, ServletHolder servletHolder, ServletContextHandler context = new ServletContextHandler(ServletContextHandler.SESSIONS); context.setContextPath(basePath); context.addServlet(servletHolder, MATCH_ALL); + context.addFilter(new FilterHolder(new CustomHeaderFilter(config)), "/*", null); for (Pair attribute : attributes) { context.setAttribute(attribute.getLeft(), attribute.getRight()); } @@ -420,5 +433,43 @@ protected void refreshSslContext() { } } + static class CustomHeaderFilter implements Filter { + + Map defaultHeaders = new HashMap<>(); + + public CustomHeaderFilter(ProxyConfiguration config) { + String headerJson = config.getProxyHttpResponseHeadersJson(); + if (StringUtils.isNotBlank(headerJson)) { + try { + defaultHeaders = ObjectMapperFactory.getMapper().getObjectMapper().readerFor(Map.class) + .readValue(headerJson); + } catch (JsonProcessingException e) { + log.warn("Failed to deserialize json headers {}", headerJson, e); + } + } + } + + @Override + public void init(FilterConfig filterConfig) throws ServletException { + } + + @Override + public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) + throws IOException, ServletException { + if (defaultHeaders != null && response instanceof HttpServletResponse) { + HttpServletResponse httpResponse = (HttpServletResponse) response; + defaultHeaders.forEach((header, value) -> { + httpResponse.setHeader(header, value); + }); + + } + chain.doFilter(request, response); + } + + @Override + public void destroy() { + } + } + private static final Logger log = LoggerFactory.getLogger(WebServer.class); } diff --git a/pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyAdditionalServletTest.java b/pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyAdditionalServletTest.java index e12224da37199..cebec2748942a 100644 --- a/pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyAdditionalServletTest.java +++ b/pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyAdditionalServletTest.java @@ -28,6 +28,7 @@ import org.apache.pulsar.client.api.Authentication; import org.apache.pulsar.client.api.AuthenticationFactory; import org.apache.pulsar.common.configuration.PulsarConfigurationLoader; +import org.apache.pulsar.common.util.ObjectMapperFactory; import org.apache.pulsar.metadata.impl.ZKMetadataStore; import org.apache.pulsar.broker.web.plugin.servlet.AdditionalServletWithClassLoader; import org.apache.pulsar.broker.web.plugin.servlet.AdditionalServlets; @@ -68,6 +69,7 @@ public class ProxyAdditionalServletTest extends MockedPulsarServiceBaseTest { private WebServer proxyWebServer; private ProxyConfiguration proxyConfig = new ProxyConfiguration(); private Authentication proxyClientAuthentication; + private Map responseHeaders = new HashMap<>(); @Override @BeforeClass @@ -82,6 +84,9 @@ protected void setup() throws Exception { // enable full parsing feature proxyConfig.setProxyLogLevel(Optional.of(2)); proxyConfig.setClusterName(configClusterName); + responseHeaders.put("header1", "value1"); + proxyConfig.setProxyHttpResponseHeadersJson( + ObjectMapperFactory.getMapper().writer().writeValueAsString(responseHeaders)); // this is for nar package test // addServletNar(); @@ -198,11 +203,15 @@ public void test() throws IOException { int httpPort = proxyWebServer.getListenPortHTTP().get(); log.info("proxy service httpPort {}", httpPort); String paramValue = "value - " + RandomUtils.nextInt(); - String response = httpGet("http://localhost:" + httpPort + BASE_PATH + "?" + QUERY_PARAM + "=" + paramValue); + final Map headers = new HashMap<>(); + String response = httpGet("http://localhost:" + httpPort + BASE_PATH + "?" + QUERY_PARAM + "=" + paramValue, + headers); Assert.assertEquals(response, paramValue); + String headerKey = "header1"; + Assert.assertEquals(headers.get(headerKey), responseHeaders.get(headerKey)); } - String httpGet(String url) throws IOException { + String httpGet(String url, Map headers) throws IOException { OkHttpClient client = new OkHttpClient(); okhttp3.Request request = new okhttp3.Request.Builder() .get() @@ -210,6 +219,9 @@ String httpGet(String url) throws IOException { .build(); try (Response response = client.newCall(request).execute()) { + response.headers().forEach(pair -> { + headers.put(pair.getFirst(), pair.getSecond()); + }); return response.body().string(); } } From eb1391a199d2c85f1ad7ce22a4d3eefa78d2fed3 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Fri, 14 Feb 2025 08:56:40 +0200 Subject: [PATCH 05/16] [improve][proxy] Make keep-alive interval configurable in Pulsar Proxy (#23981) --- conf/proxy.conf | 7 + .../proxy/server/ProxyConfiguration.java | 9 + .../pulsar/proxy/server/ProxyConnection.java | 2 +- .../proxy/server/ProxyConfigurationTest.java | 16 +- .../server/ProxyOriginalClientIPTest.java | 3 +- ...roxyServiceStarterDisableZeroCopyTest.java | 2 +- .../proxy/server/ProxyServiceStarterTest.java | 45 +++- .../server/ProxyServiceTlsStarterTest.java | 22 +- pulsar-proxy/src/test/resources/proxy.conf | 250 ------------------ 9 files changed, 76 insertions(+), 280 deletions(-) delete mode 100644 pulsar-proxy/src/test/resources/proxy.conf diff --git a/conf/proxy.conf b/conf/proxy.conf index 567cc0772a310..46d84744e1297 100644 --- a/conf/proxy.conf +++ b/conf/proxy.conf @@ -59,6 +59,13 @@ bindAddress=0.0.0.0 # If not set, the value of `InetAddress.getLocalHost().getCanonicalHostName()` is used. advertisedAddress= +# Specifies the interval (in seconds) for sending ping messages to the client. Set to 0 to disable +# ping messages. This setting applies to client connections used for topic lookups and +# partition metadata requests. When a client establishes a broker connection via the proxy, +# the client and broker will communicate directly without the proxy intercepting the messages. +# In that case, the broker's keepAliveIntervalSeconds configuration becomes relevant. +keepAliveIntervalSeconds=30 + # Enable or disable the HAProxy protocol. # If true, the real IP addresses of consumers and producers can be obtained when getting topic statistics data. haProxyProtocolEnabled=false diff --git a/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConfiguration.java b/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConfiguration.java index 329e6d52ba6f1..d89801d360b1c 100644 --- a/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConfiguration.java +++ b/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConfiguration.java @@ -266,6 +266,15 @@ public class ProxyConfiguration implements PulsarConfiguration { ) private String advertisedAddress; + @FieldContext( + category = CATEGORY_SERVER, + doc = "Specifies the interval (in seconds) for sending ping messages to the client. Set to 0 to disable " + + "ping messages. This setting applies to client connections used for topic lookups and " + + "partition metadata requests. When a client establishes a broker connection via the proxy, " + + "the client and broker will communicate directly without the proxy intercepting the messages. " + + "In that case, the broker's keepAliveIntervalSeconds configuration becomes relevant.") + private int keepAliveIntervalSeconds = 30; + @FieldContext(category = CATEGORY_SERVER, doc = "Enable or disable the proxy protocol.") private boolean haProxyProtocolEnabled; diff --git a/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConnection.java b/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConnection.java index f8b5d0844509e..540771c86fbe4 100644 --- a/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConnection.java +++ b/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConnection.java @@ -155,7 +155,7 @@ ConnectionPool getConnectionPool() { } public ProxyConnection(ProxyService proxyService, DnsAddressResolverGroup dnsAddressResolverGroup) { - super(30, TimeUnit.SECONDS); + super(proxyService.getConfiguration().getKeepAliveIntervalSeconds(), TimeUnit.SECONDS); this.service = proxyService; this.dnsAddressResolverGroup = dnsAddressResolverGroup; this.state = State.Init; diff --git a/pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyConfigurationTest.java b/pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyConfigurationTest.java index a9a562e04c899..18e7efbd7b5c6 100644 --- a/pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyConfigurationTest.java +++ b/pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyConfigurationTest.java @@ -147,7 +147,7 @@ public void testBrokerUrlCheck() throws IOException { theMock.when(PulsarConfigurationLoader.create(Mockito.anyString(), Mockito.any())) .thenReturn(configuration); try { - new ProxyServiceStarter(ProxyServiceStarterTest.ARGS); + new ProxyServiceStarter(ProxyServiceStarterTest.getArgs()); fail("brokerServiceURL must start with pulsar://"); } catch (Exception ex) { assertTrue(ex.getMessage().contains("brokerServiceURL must start with pulsar://")); @@ -161,7 +161,7 @@ public void testBrokerUrlCheck() throws IOException { theMock.when(PulsarConfigurationLoader.create(Mockito.anyString(), Mockito.any())) .thenReturn(configuration); try { - new ProxyServiceStarter(ProxyServiceStarterTest.ARGS); + new ProxyServiceStarter(ProxyServiceStarterTest.getArgs()); fail("brokerServiceURLTLS must start with pulsar+ssl://"); } catch (Exception ex) { assertTrue(ex.getMessage().contains("brokerServiceURLTLS must start with pulsar+ssl://")); @@ -174,7 +174,7 @@ public void testBrokerUrlCheck() throws IOException { theMock.when(PulsarConfigurationLoader.create(Mockito.anyString(), Mockito.any())) .thenReturn(configuration); try { - new ProxyServiceStarter(ProxyServiceStarterTest.ARGS); + new ProxyServiceStarter(ProxyServiceStarterTest.getArgs()); fail("brokerServiceURL does not support multi urls yet"); } catch (Exception ex) { assertTrue(ex.getMessage().contains("does not support multi urls yet")); @@ -188,7 +188,7 @@ public void testBrokerUrlCheck() throws IOException { theMock.when(PulsarConfigurationLoader.create(Mockito.anyString(), Mockito.any())) .thenReturn(configuration); try { - new ProxyServiceStarter(ProxyServiceStarterTest.ARGS); + new ProxyServiceStarter(ProxyServiceStarterTest.getArgs()); fail("brokerServiceURLTLS does not support multi urls yet"); } catch (Exception ex) { assertTrue(ex.getMessage().contains("does not support multi urls yet")); @@ -202,7 +202,7 @@ public void testBrokerUrlCheck() throws IOException { theMock.when(PulsarConfigurationLoader.create(Mockito.anyString(), Mockito.any())) .thenReturn(configuration); try { - new ProxyServiceStarter(ProxyServiceStarterTest.ARGS); + new ProxyServiceStarter(ProxyServiceStarterTest.getArgs()); fail("brokerWebServiceURL does not support multi urls yet"); } catch (Exception ex) { assertTrue(ex.getMessage().contains("does not support multi urls yet")); @@ -216,7 +216,7 @@ public void testBrokerUrlCheck() throws IOException { theMock.when(PulsarConfigurationLoader.create(Mockito.anyString(), Mockito.any())) .thenReturn(configuration); try { - new ProxyServiceStarter(ProxyServiceStarterTest.ARGS); + new ProxyServiceStarter(ProxyServiceStarterTest.getArgs()); fail("brokerWebServiceURLTLS does not support multi urls yet"); } catch (Exception ex) { assertTrue(ex.getMessage().contains("does not support multi urls yet")); @@ -230,7 +230,7 @@ public void testBrokerUrlCheck() throws IOException { theMock.when(PulsarConfigurationLoader.create(Mockito.anyString(), Mockito.any())) .thenReturn(configuration); try { - new ProxyServiceStarter(ProxyServiceStarterTest.ARGS); + new ProxyServiceStarter(ProxyServiceStarterTest.getArgs()); fail("functionWorkerWebServiceURL does not support multi urls yet"); } catch (Exception ex) { assertTrue(ex.getMessage().contains("does not support multi urls yet")); @@ -244,7 +244,7 @@ public void testBrokerUrlCheck() throws IOException { theMock.when(PulsarConfigurationLoader.create(Mockito.anyString(), Mockito.any())) .thenReturn(configuration); try { - new ProxyServiceStarter(ProxyServiceStarterTest.ARGS); + new ProxyServiceStarter(ProxyServiceStarterTest.getArgs()); fail("functionWorkerWebServiceURLTLS does not support multi urls yet"); } catch (Exception ex) { assertTrue(ex.getMessage().contains("does not support multi urls yet")); diff --git a/pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyOriginalClientIPTest.java b/pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyOriginalClientIPTest.java index b267439d47113..50ae6e627e820 100644 --- a/pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyOriginalClientIPTest.java +++ b/pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyOriginalClientIPTest.java @@ -39,7 +39,6 @@ @Slf4j @Test(groups = "broker") public class ProxyOriginalClientIPTest extends MockedPulsarServiceBaseTest { - static final String[] ARGS = new String[]{"-c", "./src/test/resources/proxy.conf"}; HttpClient httpClient; ProxyServiceStarter serviceStarter; String webServiceUrl; @@ -49,7 +48,7 @@ public class ProxyOriginalClientIPTest extends MockedPulsarServiceBaseTest { @BeforeClass protected void setup() throws Exception { internalSetup(); - serviceStarter = new ProxyServiceStarter(ARGS, proxyConfig -> { + serviceStarter = new ProxyServiceStarter(ProxyServiceStarterTest.getArgs(), proxyConfig -> { proxyConfig.setBrokerServiceURL(pulsar.getBrokerServiceUrl()); proxyConfig.setBrokerWebServiceURL(pulsar.getWebServiceAddress()); proxyConfig.setWebServicePort(Optional.of(0)); diff --git a/pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyServiceStarterDisableZeroCopyTest.java b/pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyServiceStarterDisableZeroCopyTest.java index 937526629acf0..b645c47242546 100644 --- a/pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyServiceStarterDisableZeroCopyTest.java +++ b/pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyServiceStarterDisableZeroCopyTest.java @@ -27,7 +27,7 @@ public class ProxyServiceStarterDisableZeroCopyTest extends ProxyServiceStarterT @BeforeClass protected void setup() throws Exception { internalSetup(); - serviceStarter = new ProxyServiceStarter(ARGS, null, true); + serviceStarter = new ProxyServiceStarter(getArgs(), null, true); serviceStarter.getConfig().setBrokerServiceURL(pulsar.getBrokerServiceUrl()); serviceStarter.getConfig().setBrokerWebServiceURL(pulsar.getWebServiceAddress()); serviceStarter.getConfig().setWebServicePort(Optional.of(0)); diff --git a/pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyServiceStarterTest.java b/pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyServiceStarterTest.java index d96d2cd1f6e9c..6ef24874387a8 100644 --- a/pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyServiceStarterTest.java +++ b/pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyServiceStarterTest.java @@ -21,16 +21,23 @@ import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertTrue; import static org.testng.Assert.fail; +import java.io.File; +import java.io.FileInputStream; +import java.io.FileOutputStream; import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; import java.net.URI; import java.nio.ByteBuffer; import java.util.Base64; import java.util.Map; import java.util.Optional; +import java.util.Properties; import java.util.concurrent.ArrayBlockingQueue; import java.util.concurrent.Future; import java.util.function.Consumer; import lombok.Cleanup; +import lombok.SneakyThrows; import org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest; import org.apache.pulsar.client.api.Authentication; import org.apache.pulsar.client.api.Producer; @@ -50,17 +57,38 @@ import org.testng.annotations.Test; public class ProxyServiceStarterTest extends MockedPulsarServiceBaseTest { - - public static final String[] ARGS = new String[]{"-c", "./src/test/resources/proxy.conf"}; - protected ProxyServiceStarter serviceStarter; protected String serviceUrl; + private static File proxyConfFileForTests; + + @SneakyThrows + public static String[] getArgs() { + if (proxyConfFileForTests == null) { + // load the properties from the proxy.conf file + Properties properties = new Properties(); + try (InputStream inputStream = new FileInputStream("../conf/proxy.conf")) { + properties.load(inputStream); + } + // set dummy values for the required properties so that validation is passed + properties.setProperty("brokerServiceURL", "pulsar://0.0.0.0:0"); + properties.setProperty("brokerWebServiceURL", "http://0.0.0.0:0"); + // change keepAliveIntervalSeconds default value so that it's possible to validate that it's configured + properties.setProperty("keepAliveIntervalSeconds", "25"); + // write the properties to a temporary file + proxyConfFileForTests = File.createTempFile("proxy", ".conf"); + proxyConfFileForTests.deleteOnExit(); + try (OutputStream out = new FileOutputStream(proxyConfFileForTests)) { + properties.store(out, null); + } + } + return new String[] { "-c", proxyConfFileForTests.getAbsolutePath() }; + } @Override @BeforeClass protected void setup() throws Exception { internalSetup(); - serviceStarter = new ProxyServiceStarter(ARGS, null, true); + serviceStarter = new ProxyServiceStarter(getArgs(), null, true); serviceStarter.getConfig().setBrokerServiceURL(pulsar.getBrokerServiceUrl()); serviceStarter.getConfig().setBrokerWebServiceURL(pulsar.getWebServiceAddress()); serviceStarter.getConfig().setWebServicePort(Optional.of(0)); @@ -100,6 +128,11 @@ public void testProducer() throws Exception { } } + @Test + public void testKeepAliveIntervalSecondsIsConfigured() throws Exception { + assertEquals(serviceStarter.getConfig().getKeepAliveIntervalSeconds(), 25); + } + @Test public void testProduceAndConsumeMessageWithWebsocket() throws Exception { @Cleanup("stop") @@ -180,7 +213,7 @@ public void testProxyClientAuthentication() throws Exception { - ProxyServiceStarter serviceStarter = new ProxyServiceStarter(ARGS, null, true); + ProxyServiceStarter serviceStarter = new ProxyServiceStarter(getArgs(), null, true); initConfig.accept(serviceStarter.getConfig()); // ProxyServiceStarter will throw an exception when Authentication#start is failed serviceStarter.getConfig().setBrokerClientAuthenticationPlugin(ExceptionAuthentication1.class.getName()); @@ -192,7 +225,7 @@ public void testProxyClientAuthentication() throws Exception { assertTrue(serviceStarter.getProxyClientAuthentication() instanceof ExceptionAuthentication1); } - serviceStarter = new ProxyServiceStarter(ARGS, null, true); + serviceStarter = new ProxyServiceStarter(getArgs(), null, true); initConfig.accept(serviceStarter.getConfig()); // ProxyServiceStarter will throw an exception when Authentication#start and Authentication#close are failed serviceStarter.getConfig().setBrokerClientAuthenticationPlugin(ExceptionAuthentication2.class.getName()); diff --git a/pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyServiceTlsStarterTest.java b/pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyServiceTlsStarterTest.java index ee8ae8d4afb3c..3e1c1200a747a 100644 --- a/pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyServiceTlsStarterTest.java +++ b/pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyServiceTlsStarterTest.java @@ -18,6 +18,15 @@ */ package org.apache.pulsar.proxy.server; +import static org.apache.pulsar.proxy.server.ProxyServiceStarterTest.getArgs; +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertTrue; +import java.net.URI; +import java.nio.ByteBuffer; +import java.util.Base64; +import java.util.Optional; +import java.util.concurrent.ArrayBlockingQueue; +import java.util.concurrent.Future; import lombok.Cleanup; import org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest; import org.apache.pulsar.client.api.Producer; @@ -35,17 +44,6 @@ import org.testng.annotations.BeforeClass; import org.testng.annotations.Test; -import java.net.URI; -import java.nio.ByteBuffer; -import java.util.Base64; -import java.util.Optional; -import java.util.concurrent.ArrayBlockingQueue; -import java.util.concurrent.Future; - -import static org.apache.pulsar.proxy.server.ProxyServiceStarterTest.ARGS; -import static org.testng.Assert.assertEquals; -import static org.testng.Assert.assertTrue; - public class ProxyServiceTlsStarterTest extends MockedPulsarServiceBaseTest { private ProxyServiceStarter serviceStarter; private String serviceUrl; @@ -55,7 +53,7 @@ public class ProxyServiceTlsStarterTest extends MockedPulsarServiceBaseTest { @BeforeClass protected void setup() throws Exception { internalSetup(); - serviceStarter = new ProxyServiceStarter(ARGS, null, true); + serviceStarter = new ProxyServiceStarter(getArgs(), null, true); serviceStarter.getConfig().setBrokerServiceURL(pulsar.getBrokerServiceUrl()); serviceStarter.getConfig().setBrokerServiceURLTLS(pulsar.getBrokerServiceUrlTls()); serviceStarter.getConfig().setBrokerWebServiceURL(pulsar.getWebServiceAddress()); diff --git a/pulsar-proxy/src/test/resources/proxy.conf b/pulsar-proxy/src/test/resources/proxy.conf deleted file mode 100644 index aec9f5ee1c5e9..0000000000000 --- a/pulsar-proxy/src/test/resources/proxy.conf +++ /dev/null @@ -1,250 +0,0 @@ -# -# 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. -# - -### --- Broker Discovery --- ### - -# The metadata store URL -# Examples: -# * zk:my-zk-1:2181,my-zk-2:2181,my-zk-3:2181 -# * my-zk-1:2181,my-zk-2:2181,my-zk-3:2181 (will default to ZooKeeper when the schema is not specified) -# * zk:my-zk-1:2181,my-zk-2:2181,my-zk-3:2181/my-chroot-path (to add a ZK chroot path) -metadataStoreUrl= - -# The metadata store URL for the configuration data. If empty, we fall back to use metadataStoreUrl -configurationMetadataStoreUrl= - -# if Service Discovery is Disabled this url should point to the discovery service provider. -brokerServiceURL=pulsar://0.0.0.0:0 -brokerServiceURLTLS= - -# These settings are unnecessary if `zookeeperServers` is specified -brokerWebServiceURL=http://0.0.0.0:0 -brokerWebServiceURLTLS= - -# If function workers are setup in a separate cluster, configure the following 2 settings -# to point to the function workers cluster -functionWorkerWebServiceURL= -functionWorkerWebServiceURLTLS= - -# ZooKeeper session timeout (in milliseconds) -zookeeperSessionTimeoutMs=30000 - -# ZooKeeper cache expiry time in seconds -zooKeeperCacheExpirySeconds=300 - -### --- Server --- ### - -# Hostname or IP address the service binds on, default is 0.0.0.0. -bindAddress=0.0.0.0 - -# Hostname or IP address the service advertises to the outside world. -# If not set, the value of `InetAddress.getLocalHost().getCanonicalHostName()` is used. -advertisedAddress= - -# Enable or disable the HAProxy protocol. -haProxyProtocolEnabled=false - -# Enables zero-copy transport of data across network interfaces using the splice system call. -# Zero copy mode cannot be used when TLS is enabled or when proxyLogLevel is > 0. -proxyZeroCopyModeEnabled=true - -# The port to use for server binary Protobuf requests -servicePort=6650 - -# The port to use to server binary Protobuf TLS requests -servicePortTls= - -# Port that discovery service listen on -webServicePort=8080 - -# Port to use to server HTTPS request -webServicePortTls= - -# Path for the file used to determine the rotation status for the proxy instance when responding -# to service discovery health checks -statusFilePath= - -# Proxy log level, default is 0. -# 0: Do not log any tcp channel info -# 1: Parse and log any tcp channel info and command info without message body -# 2: Parse and log channel info, command info and message body -proxyLogLevel=0 - -### ---Authorization --- ### - -# Role names that are treated as "super-users," meaning that they will be able to perform all admin -# operations and publish/consume to/from all topics (as a comma-separated list) -superUserRoles= - -# Whether authorization is enforced by the Pulsar proxy -authorizationEnabled=false - -# Authorization provider as a fully qualified class name -authorizationProvider=org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider - -# Whether client authorization credentials are forwared to the broker for re-authorization. -# Authentication must be enabled via authenticationEnabled=true for this to take effect. -forwardAuthorizationCredentials=false - -### --- Authentication --- ### - -# Whether authentication is enabled for the Pulsar proxy -authenticationEnabled=false - -# Authentication provider name list (a comma-separated list of class names) -authenticationProviders= - -# When this parameter is not empty, unauthenticated users perform as anonymousUserRole -anonymousUserRole= - -### --- Client Authentication --- ### - -# The three brokerClient* authentication settings below are for the proxy itself and determine how it -# authenticates with Pulsar brokers - -# The authentication plugin used by the Pulsar proxy to authenticate with Pulsar brokers -brokerClientAuthenticationPlugin= - -# The authentication parameters used by the Pulsar proxy to authenticate with Pulsar brokers -brokerClientAuthenticationParameters= - -# The path to trusted certificates used by the Pulsar proxy to authenticate with Pulsar brokers -brokerClientTrustCertsFilePath= - -# Whether TLS is enabled when communicating with Pulsar brokers -tlsEnabledWithBroker=false - -# Tls cert refresh duration in seconds (set 0 to check on every new connection) -tlsCertRefreshCheckDurationSec=300 - -##### --- Rate Limiting --- ##### - -# Max concurrent inbound connections. The proxy will reject requests beyond that. -maxConcurrentInboundConnections=10000 - -# Max concurrent outbound connections. The proxy will error out requests beyond that. -maxConcurrentLookupRequests=50000 - -##### --- TLS --- ##### - -# Deprecated - use servicePortTls and webServicePortTls instead -tlsEnabledInProxy=false - -# Path for the TLS certificate file -tlsCertificateFilePath= - -# Path for the TLS private key file -tlsKeyFilePath= - -# Path for the trusted TLS certificate file. -# This cert is used to verify that any certs presented by connecting clients -# are signed by a certificate authority. If this verification -# fails, then the certs are untrusted and the connections are dropped. -tlsTrustCertsFilePath= - -# Accept untrusted TLS certificate from client. -# If true, a client with a cert which cannot be verified with the -# 'tlsTrustCertsFilePath' cert will allowed to connect to the server, -# though the cert will not be used for client authentication. -tlsAllowInsecureConnection=false - -# Whether the hostname is validated when the proxy creates a TLS connection with brokers -tlsHostnameVerificationEnabled=false - -# Specify the tls protocols the broker will use to negotiate during TLS handshake -# (a comma-separated list of protocol names). -# Examples:- [TLSv1.3, TLSv1.2] -tlsProtocols= - -# Specify the tls cipher the broker will use to negotiate during TLS Handshake -# (a comma-separated list of ciphers). -# Examples:- [TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256] -tlsCiphers= - -# Whether client certificates are required for TLS. Connections are rejected if the client -# certificate isn't trusted. -tlsRequireTrustedClientCertOnConnect=false - -##### --- HTTP --- ##### - -# Http directs to redirect to non-pulsar services. -httpReverseProxyConfigs= - -# Http output buffer size. The amount of data that will be buffered for http requests -# before it is flushed to the channel. A larger buffer size may result in higher http throughput -# though it may take longer for the client to see data. -# If using HTTP streaming via the reverse proxy, this should be set to the minimum value, 1, -# so that clients see the data as soon as possible. -httpOutputBufferSize=32768 - -# Number of threads to use for HTTP requests processing. Default is -# 2 * Runtime.getRuntime().availableProcessors() -httpNumThreads= - -# Enable the enforcement of limits on the incoming HTTP requests -httpRequestsLimitEnabled=false - -# Max HTTP requests per seconds allowed. The excess of requests will be rejected with HTTP code 429 (Too many requests) -httpRequestsMaxPerSecond=100.0 - - -### --- Token Authentication Provider --- ### - -## Symmetric key -# Configure the secret key to be used to validate auth tokens -# The key can be specified like: -# tokenSecretKey=data:;base64,xxxxxxxxx -# tokenSecretKey=file:///my/secret.key ( Note: key file must be DER-encoded ) -tokenSecretKey= - -## Asymmetric public/private key pair -# Configure the public key to be used to validate auth tokens -# The key can be specified like: -# tokenPublicKey=data:;base64,xxxxxxxxx -# tokenPublicKey=file:///my/public.key ( Note: key file must be DER-encoded ) -tokenPublicKey= - -# The token "claim" that will be interpreted as the authentication "role" or "principal" by AuthenticationProviderToken (defaults to "sub" if blank) -tokenAuthClaim= - -# The token audience "claim" name, e.g. "aud", that will be used to get the audience from token. -# If not set, audience will not be verified. -tokenAudienceClaim= - -# The token audience stands for this broker. The field `tokenAudienceClaim` of a valid token, need contains this. -tokenAudience= - -### --- WebSocket config variables --- ### - -# Enable or disable the WebSocket servlet. -webSocketServiceEnabled=false - -# Name of the cluster to which this broker belongs to -clusterName= - -### --- Deprecated config variables --- ### - -# Deprecated. Use configurationStoreServers -globalZookeeperServers= - -# The ZooKeeper quorum connection string (as a comma-separated list) -zookeeperServers= - -# Configuration store connection string (as a comma-separated list) -configurationStoreServers= From 5443c69d84818cb4a49704f7ab7dbccf65b2179a Mon Sep 17 00:00:00 2001 From: Jiawen Wang <74594733+summeriiii@users.noreply.github.com> Date: Fri, 14 Feb 2025 17:56:45 +0800 Subject: [PATCH 06/16] [fix][broker] Fix incorrect blockedConsumerOnUnackedMsgs value when maxUnackedMessagesPerConsumer is 1 (#23796) --- .../pulsar/broker/service/Consumer.java | 7 +++- .../broker/service/BrokerServiceTest.java | 41 +++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Consumer.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Consumer.java index b46e10a20fd34..61f9d5c86b32f 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Consumer.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Consumer.java @@ -595,6 +595,7 @@ private CompletableFuture individualAckNormal(CommandAck ack, Map // consumer can start again consuming messages int unAckedMsgs = UNACKED_MESSAGES_UPDATER.get(ackOwnedConsumer); @@ -1090,7 +1096,6 @@ private boolean removePendingAcks(Consumer ackOwnedConsumer, Position position) ackOwnedConsumer.blockedConsumerOnUnackedMsgs = false; flowConsumerBlockedPermits(ackOwnedConsumer); } - return true; } public PendingAcksMap getPendingAcks() { diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerServiceTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerServiceTest.java index 89727014be99e..fa76fdd5bf45c 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerServiceTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerServiceTest.java @@ -1790,6 +1790,47 @@ public void testDuplicateAcknowledgement() throws Exception { .get("sub-1").getUnackedMessages(), 0); } + @Test + public void testBlockedConsumerOnUnackedMsgs() throws Exception { + final String ns = "prop/ns-test"; + admin.namespaces().createNamespace(ns, 2); + admin.namespaces().setMaxUnackedMessagesPerConsumer(ns, 1); + + final String topicName = "persistent://prop/ns-test/testBlockedConsumerOnUnackedMsgs"; + @Cleanup + Producer producer = pulsarClient.newProducer() + .topic(topicName) + .create(); + @Cleanup + Consumer consumer = pulsarClient.newConsumer() + .topic(topicName) + .subscriptionName("sub-test") + .acknowledgmentGroupTime(0, TimeUnit.SECONDS) + .subscriptionType(SubscriptionType.Shared) + .isAckReceiptEnabled(true) + .receiverQueueSize(0) + .subscribe(); + + producer.send("1".getBytes(StandardCharsets.UTF_8)); + producer.send("2".getBytes(StandardCharsets.UTF_8)); + + // 1. receive message + Message message = consumer.receive(); + Thread.sleep(ASYNC_EVENT_COMPLETION_WAIT); + + SubscriptionStats subscriptionStats = admin.topics().getStats(topicName).getSubscriptions().get("sub-test"); + assertEquals(subscriptionStats.getUnackedMessages(), 1); + assertTrue(subscriptionStats.getConsumers().get(0).isBlockedConsumerOnUnackedMsgs()); + + // 2、ack this message + consumer.acknowledge(message); + Thread.sleep(ASYNC_EVENT_COMPLETION_WAIT); + + subscriptionStats = admin.topics().getStats(topicName).getSubscriptions().get("sub-test"); + assertEquals(subscriptionStats.getUnackedMessages(), 0); + assertFalse(subscriptionStats.getConsumers().get(0).isBlockedConsumerOnUnackedMsgs()); + } + @Test public void testUnsubscribeNonDurableSub() throws Exception { final String ns = "prop/ns-test"; From 40d5af4a43ca508ef496367925868310ea683780 Mon Sep 17 00:00:00 2001 From: Heesung Sohn <103456639+heesung-sn@users.noreply.github.com> Date: Fri, 14 Feb 2025 02:52:13 -0800 Subject: [PATCH 07/16] [fix][meta] Fix ephemeral Zookeeper put which creates a persistent znode (#23984) --- .../pulsar/metadata/impl/ZKMetadataStore.java | 4 +-- .../metadata/MetadataStoreExtendedTest.java | 34 +++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/ZKMetadataStore.java b/pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/ZKMetadataStore.java index 4c24aa5938b93..8fd8252152898 100644 --- a/pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/ZKMetadataStore.java +++ b/pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/ZKMetadataStore.java @@ -439,8 +439,8 @@ private void internalStorePut(OpPut opPut) { future.completeExceptionally(getException(Code.BADVERSION, opPut.getPath())); } else { // The z-node does not exist, let's create it first - put(opPut.getPath(), opPut.getData(), Optional.of(-1L)).thenAccept( - s -> future.complete(s)) + put(opPut.getPath(), opPut.getData(), Optional.of(-1L), opPut.getOptions()) + .thenAccept(s -> future.complete(s)) .exceptionally(ex -> { if (ex.getCause() instanceof BadVersionException) { // The z-node exist now, let's overwrite it diff --git a/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataStoreExtendedTest.java b/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataStoreExtendedTest.java index 9a38cdbcd2f85..a4c937611fd3f 100644 --- a/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataStoreExtendedTest.java +++ b/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataStoreExtendedTest.java @@ -66,4 +66,38 @@ public void sequentialKeys(String provider, Supplier urlSupplier) throws assertNotEquals(seq1, seq2); assertTrue(n1 < n2); } + + @Test(dataProvider = "impl") + public void testPersistentOrEphemeralPut(String provider, Supplier urlSupplier) throws Exception { + final String key1 = newKey(); + MetadataStoreExtended store = MetadataStoreExtended.create(urlSupplier.get(), MetadataStoreConfig.builder().build()); + store.put(key1, "value-1".getBytes(), Optional.empty(), EnumSet.noneOf(CreateOption.class)).join(); + var value = store.get(key1).join().get(); + assertEquals(value.getValue(), "value-1".getBytes()); + // assertFalse(value.getStat().isEphemeral()); // Todo : fix zkStat.getEphemeralOwner() != 0 from test zk + assertTrue(value.getStat().isFirstVersion()); + var version = value.getStat().getVersion(); + + store.put(key1, "value-2".getBytes(), Optional.empty(), EnumSet.noneOf(CreateOption.class)).join(); + value = store.get(key1).join().get(); + assertEquals(value.getValue(), "value-2".getBytes()); + //assertFalse(value.getStat().isEphemeral()); // Todo : fix zkStat.getEphemeralOwner() != 0 from test zk + assertEquals(value.getStat().getVersion(), version + 1); + + final String key2 = newKey(); + store.put(key2, "value-4".getBytes(), Optional.empty(), EnumSet.of(CreateOption.Ephemeral)).join(); + value = store.get(key2).join().get(); + assertEquals(value.getValue(), "value-4".getBytes()); + assertTrue(value.getStat().isEphemeral()); + assertTrue(value.getStat().isFirstVersion()); + version = value.getStat().getVersion(); + + + store.put(key2, "value-5".getBytes(), Optional.empty(), EnumSet.of(CreateOption.Ephemeral)).join(); + value = store.get(key2).join().get(); + assertEquals(value.getValue(), "value-5".getBytes()); + assertTrue(value.getStat().isEphemeral()); + assertEquals(value.getStat().getVersion(), version + 1); + } + } From eb7a4f36da3711b9b93527d0bfe26acad1d64b1d Mon Sep 17 00:00:00 2001 From: Wenzhi Feng <52550727+thetumbled@users.noreply.github.com> Date: Fri, 14 Feb 2025 19:18:06 +0800 Subject: [PATCH 08/16] [fix][test] fix flaky testNegativeAcksWithBackoff when batch enabled. (#23986) --- .../org/apache/pulsar/client/impl/NegativeAcksTest.java | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/client/impl/NegativeAcksTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/client/impl/NegativeAcksTest.java index 7ab3e545e981e..182c952eac82d 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/client/impl/NegativeAcksTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/client/impl/NegativeAcksTest.java @@ -256,9 +256,16 @@ public void testNegativeAcksWithBackoff(boolean batching, boolean usePartitions, long firstReceivedAt = System.currentTimeMillis(); long expectedTotalRedeliveryDelay = 0; for (int i = 0; i < redeliverCount; i++) { + Message msg = null; for (int j = 0; j < N; j++) { - Message msg = consumer.receive(); + msg = consumer.receive(); log.info("Received message {}", msg.getValue()); + if (!batching) { + consumer.negativeAcknowledge(msg); + } + } + if (batching) { + // for batching, we only need to nack one message in the batch to trigger redelivery consumer.negativeAcknowledge(msg); } expectedTotalRedeliveryDelay += backoff.next(i); From 58120841ec74373b5ea718dd0325d4542dde4330 Mon Sep 17 00:00:00 2001 From: Matteo Merli Date: Fri, 14 Feb 2025 08:54:59 -0800 Subject: [PATCH 09/16] [fix] fix for code scanning alert no. 48: Uncontrolled data used in path expression (#23985) Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- .../storage/filesystem/FileSystemPackagesStorage.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/pulsar-package-management/filesystem-storage/src/main/java/org/apache/pulsar/packages/management/storage/filesystem/FileSystemPackagesStorage.java b/pulsar-package-management/filesystem-storage/src/main/java/org/apache/pulsar/packages/management/storage/filesystem/FileSystemPackagesStorage.java index 2bb43bb207203..8bf7851fc8d63 100644 --- a/pulsar-package-management/filesystem-storage/src/main/java/org/apache/pulsar/packages/management/storage/filesystem/FileSystemPackagesStorage.java +++ b/pulsar-package-management/filesystem-storage/src/main/java/org/apache/pulsar/packages/management/storage/filesystem/FileSystemPackagesStorage.java @@ -59,11 +59,14 @@ public class FileSystemPackagesStorage implements PackagesStorage { } private File getPath(String path) throws IOException { - if (path.contains("..")) { + // Normalize the path to remove any redundant path elements + File f = Paths.get(storagePath.toString(), path).normalize().toFile(); + + // Ensure the normalized path is still within the storagePath + if (!f.getAbsolutePath().startsWith(storagePath.getAbsolutePath())) { throw new IOException("Invalid path: " + path); } - File f = Paths.get(storagePath.toString(), path).toFile(); if (!f.getParentFile().exists()) { if (!f.getParentFile().mkdirs()) { throw new RuntimeException("Failed to create parent dirs for " + path); From 1d53d3618b9f72f8c66dad6fc2f6248d6d79d2c1 Mon Sep 17 00:00:00 2001 From: ken <1647023764@qq.com> Date: Sat, 15 Feb 2025 07:30:56 +0800 Subject: [PATCH 10/16] [fix][broker] fix broker may lost rack information (#23331) Co-authored-by: fanjianye --- .../bookkeeper/PulsarRegistrationClient.java | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarRegistrationClient.java b/pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarRegistrationClient.java index be945d988fb88..89dbf2be990b0 100644 --- a/pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarRegistrationClient.java +++ b/pulsar-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarRegistrationClient.java @@ -181,8 +181,13 @@ private CompletableFuture>> getBookiesThenFreshCache(Str @Override public CompletableFuture watchWritableBookies(RegistrationListener registrationListener) { writableBookiesWatchers.add(registrationListener); + // trigger all listeners in writableBookiesWatchers one by one. It aims to keep a sync way + // to make sure the previous listener has finished when a new listener is register. + // Though it would bring duplicate trigger listener problem, but since watchWritableBookies + // is only executed when bookieClient construct, the duplicate problem is acceptable. return getWritableBookies() - .thenAcceptAsync(registrationListener::onBookiesChanged, executor); + .thenAcceptAsync(bookies -> + writableBookiesWatchers.forEach(w -> w.onBookiesChanged(bookies)), executor); } @Override @@ -193,8 +198,13 @@ public void unwatchWritableBookies(RegistrationListener registrationListener) { @Override public CompletableFuture watchReadOnlyBookies(RegistrationListener registrationListener) { readOnlyBookiesWatchers.add(registrationListener); + // trigger all listeners in readOnlyBookiesWatchers one by one. It aims to keep a sync way + // to make sure the previous listener has finished when a new listener is register. + // Though it would bring duplicate trigger listener problem, but since watchReadOnlyBookies + // is only executed when bookieClient construct, the duplicate problem is acceptable. return getReadOnlyBookies() - .thenAcceptAsync(registrationListener::onBookiesChanged, executor); + .thenAcceptAsync(bookies -> + readOnlyBookiesWatchers.forEach(w -> w.onBookiesChanged(bookies)), executor); } @Override From 1220951ac74fb4742abbbd331d6e751234c47015 Mon Sep 17 00:00:00 2001 From: Xiangying Meng <55571188+liangyepianzhou@users.noreply.github.com> Date: Mon, 17 Feb 2025 10:16:49 +0800 Subject: [PATCH 11/16] [improve][client][PIP-389] Add a producer config to improve compression performance (#23525) PIP: https://github.com/apache/pulsar/pull/23526 ### Motivation The motivation of this PIP is to provide a way to improve the compression performance by skipping the compression of small messages. We want to add a new configuration compressMinMsgBodySize to the producer configuration. This configuration will allow the user to set the minimum size of the message body that will be compressed. If the message body size is less than the compressMinMsgBodySize, the message will not be compressed. --- .../impl/RawBatchMessageContainerImpl.java | 2 +- .../broker/admin/PersistentTopicsTest.java | 3 +- .../impl/ProducerConsumerInternalTest.java | 48 ++++++++++++++++- .../client/impl/ProducerMemoryLeakTest.java | 1 + .../impl/BatchMessageContainerImpl.java | 32 ++++++++--- .../pulsar/client/impl/MessageImpl.java | 5 ++ .../pulsar/client/impl/ProducerImpl.java | 54 +++++++++++-------- .../impl/conf/ProducerConfigurationData.java | 2 + 8 files changed, 115 insertions(+), 32 deletions(-) diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/client/impl/RawBatchMessageContainerImpl.java b/pulsar-broker/src/main/java/org/apache/pulsar/client/impl/RawBatchMessageContainerImpl.java index 374f1e30c0a89..9b9f79a8ec5ea 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/client/impl/RawBatchMessageContainerImpl.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/client/impl/RawBatchMessageContainerImpl.java @@ -167,7 +167,7 @@ public ByteBuf toByteBuf() { } } - ByteBuf encryptedPayload = encrypt(getCompressedBatchMetadataAndPayload()); + ByteBuf encryptedPayload = encrypt(getCompressedBatchMetadataAndPayload(false)); updateAndReserveBatchAllocatedSize(encryptedPayload.capacity()); ByteBuf metadataAndPayload = Commands.serializeMetadataAndPayload(Commands.ChecksumType.Crc32c, messageMetadata, encryptedPayload); diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java index 258c0183114fd..27f9d72413c52 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java @@ -1257,11 +1257,12 @@ public void testExamineMessageMetadata() throws Exception { admin.topics().createPartitionedTopic(topicName, 2); @Cleanup - Producer producer = pulsarClient.newProducer(Schema.STRING) + ProducerImpl producer = (ProducerImpl) pulsarClient.newProducer(Schema.STRING) .producerName("testExamineMessageMetadataProducer") .compressionType(CompressionType.LZ4) .topic(topicName + "-partition-0") .create(); + producer.getConfiguration().setCompressMinMsgBodySize(1); producer.newMessage() .keyBytes("partition123".getBytes()) diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/client/impl/ProducerConsumerInternalTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/client/impl/ProducerConsumerInternalTest.java index a06085d3d4626..4617f631207f7 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/client/impl/ProducerConsumerInternalTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/client/impl/ProducerConsumerInternalTest.java @@ -18,23 +18,26 @@ */ package org.apache.pulsar.client.impl; -import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertFalse; import static org.testng.Assert.assertNotNull; +import static org.testng.Assert.assertEquals; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; + import lombok.Cleanup; import lombok.extern.slf4j.Slf4j; import org.apache.pulsar.broker.BrokerTestUtil; import org.apache.pulsar.broker.service.ServerCnx; import org.apache.pulsar.client.api.BatcherBuilder; +import org.apache.pulsar.client.api.CompressionType; import org.apache.pulsar.client.api.Consumer; import org.apache.pulsar.client.api.Message; import org.apache.pulsar.client.api.MessageId; import org.apache.pulsar.client.api.Producer; import org.apache.pulsar.client.api.ProducerConsumerBase; +import org.apache.pulsar.client.api.PulsarClientException; import org.apache.pulsar.client.api.SubscriptionType; import org.apache.pulsar.common.api.proto.CommandCloseProducer; import org.apache.pulsar.common.policies.data.PersistentTopicInternalStats; @@ -230,4 +233,47 @@ public void testRetentionPolicyByProducingMessages() throws Exception { assertEquals(internalStats.ledgers.size(), 1); }); } + + + @Test + public void testProducerCompressionMinMsgBodySize() throws PulsarClientException { + byte[] msg1022 = new byte[1022]; + byte[] msg1025 = new byte[1025]; + final String topicName = BrokerTestUtil.newUniqueName("persistent://my-property/my-ns/tp_"); + @Cleanup + ProducerImpl producer = (ProducerImpl) pulsarClient.newProducer() + .topic(topicName) + .producerName("producer") + .compressionType(CompressionType.LZ4) + .create(); + @Cleanup + Consumer consumer = pulsarClient.newConsumer() + .topic(topicName) + .subscriptionName("sub") + .subscribe(); + + producer.conf.setCompressMinMsgBodySize(1024); + producer.conf.setCompressionType(CompressionType.LZ4); + // disable batch + producer.conf.setBatchingEnabled(false); + producer.newMessage().value(msg1022).send(); + MessageImpl message = (MessageImpl) consumer.receive(); + CompressionType compressionType = message.getCompressionType(); + assertEquals(compressionType, CompressionType.NONE); + producer.newMessage().value(msg1025).send(); + message = (MessageImpl) consumer.receive(); + compressionType = message.getCompressionType(); + assertEquals(compressionType, CompressionType.LZ4); + + // enable batch + producer.conf.setBatchingEnabled(true); + producer.newMessage().value(msg1022).send(); + message = (MessageImpl) consumer.receive(); + compressionType = message.getCompressionType(); + assertEquals(compressionType, CompressionType.NONE); + producer.newMessage().value(msg1025).send(); + message = (MessageImpl) consumer.receive(); + compressionType = message.getCompressionType(); + assertEquals(compressionType, CompressionType.LZ4); + } } diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/client/impl/ProducerMemoryLeakTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/client/impl/ProducerMemoryLeakTest.java index dcdfd136476c3..e366b232a639d 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/client/impl/ProducerMemoryLeakTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/client/impl/ProducerMemoryLeakTest.java @@ -124,6 +124,7 @@ public void testSendMessageSizeExceeded(int maxMessageSize, CompressionType comp .compressionType(compressionType) .enableBatching(false) .create(); + producer.getConfiguration().setCompressMinMsgBodySize(1); producer.getConnectionHandler().setMaxMessageSize(maxMessageSize); MsgPayloadTouchableMessageBuilder msgBuilder = newMessage(producer); /** diff --git a/pulsar-client/src/main/java/org/apache/pulsar/client/impl/BatchMessageContainerImpl.java b/pulsar-client/src/main/java/org/apache/pulsar/client/impl/BatchMessageContainerImpl.java index 403a804b605e7..9e0eeafc47841 100644 --- a/pulsar-client/src/main/java/org/apache/pulsar/client/impl/BatchMessageContainerImpl.java +++ b/pulsar-client/src/main/java/org/apache/pulsar/client/impl/BatchMessageContainerImpl.java @@ -142,6 +142,10 @@ public boolean add(MessageImpl msg, SendCallback callback) { } protected ByteBuf getCompressedBatchMetadataAndPayload() { + return getCompressedBatchMetadataAndPayload(true); + } + + protected ByteBuf getCompressedBatchMetadataAndPayload(boolean clientOperation) { int batchWriteIndex = batchedMessageMetadataAndPayload.writerIndex(); int batchReadIndex = batchedMessageMetadataAndPayload.readerIndex(); @@ -169,11 +173,23 @@ protected ByteBuf getCompressedBatchMetadataAndPayload() { } int uncompressedSize = batchedMessageMetadataAndPayload.readableBytes(); - ByteBuf compressedPayload = compressor.encode(batchedMessageMetadataAndPayload); - batchedMessageMetadataAndPayload.release(); - if (compressionType != CompressionType.NONE) { - messageMetadata.setCompression(compressionType); - messageMetadata.setUncompressedSize(uncompressedSize); + ByteBuf compressedPayload; + if (clientOperation && producer != null){ + if (compressionType != CompressionType.NONE + && uncompressedSize > producer.conf.getCompressMinMsgBodySize()) { + compressedPayload = producer.applyCompression(batchedMessageMetadataAndPayload); + messageMetadata.setCompression(compressionType); + messageMetadata.setUncompressedSize(uncompressedSize); + } else { + compressedPayload = batchedMessageMetadataAndPayload; + } + } else { + compressedPayload = compressor.encode(batchedMessageMetadataAndPayload); + batchedMessageMetadataAndPayload.release(); + if (compressionType != CompressionType.NONE) { + messageMetadata.setCompression(compressionType); + messageMetadata.setUncompressedSize(uncompressedSize); + } } // Update the current max batch size using the uncompressed size, which is what we need in any case to @@ -252,7 +268,8 @@ public OpSendMsg createOpSendMsg() throws IOException { if (messages.size() == 1) { messageMetadata.clear(); messageMetadata.copyFrom(messages.get(0).getMessageBuilder()); - ByteBuf encryptedPayload = producer.encryptMessage(messageMetadata, getCompressedBatchMetadataAndPayload()); + ByteBuf encryptedPayload = producer.encryptMessage(messageMetadata, + getCompressedBatchMetadataAndPayload()); updateAndReserveBatchAllocatedSize(encryptedPayload.capacity()); ByteBufPair cmd = producer.sendMessage(producer.producerId, messageMetadata.getSequenceId(), 1, null, messageMetadata, encryptedPayload); @@ -284,7 +301,8 @@ public OpSendMsg createOpSendMsg() throws IOException { lowestSequenceId = -1L; return op; } - ByteBuf encryptedPayload = producer.encryptMessage(messageMetadata, getCompressedBatchMetadataAndPayload()); + ByteBuf encryptedPayload = producer.encryptMessage(messageMetadata, + getCompressedBatchMetadataAndPayload()); updateAndReserveBatchAllocatedSize(encryptedPayload.capacity()); if (encryptedPayload.readableBytes() > getMaxMessageSize()) { encryptedPayload.release(); diff --git a/pulsar-client/src/main/java/org/apache/pulsar/client/impl/MessageImpl.java b/pulsar-client/src/main/java/org/apache/pulsar/client/impl/MessageImpl.java index 72a5fd54e852b..aa98df6cda944 100644 --- a/pulsar-client/src/main/java/org/apache/pulsar/client/impl/MessageImpl.java +++ b/pulsar-client/src/main/java/org/apache/pulsar/client/impl/MessageImpl.java @@ -38,6 +38,7 @@ import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; import lombok.Getter; +import org.apache.pulsar.client.api.CompressionType; import org.apache.pulsar.client.api.Message; import org.apache.pulsar.client.api.MessageId; import org.apache.pulsar.client.api.MessageIdAdv; @@ -780,6 +781,10 @@ int getUncompressedSize() { return uncompressedSize; } + CompressionType getCompressionType() { + return CompressionType.valueOf(msgMetadata.getCompression().name()); + } + SchemaState getSchemaState() { return schemaState; } diff --git a/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java b/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java index fb2246f3a66a1..935a6251ddaf8 100644 --- a/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java +++ b/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java @@ -486,7 +486,8 @@ CompletableFuture internalSendWithTxnAsync(Message message, Transa * @param payload * @return a new payload */ - private ByteBuf applyCompression(ByteBuf payload) { + @VisibleForTesting + public ByteBuf applyCompression(ByteBuf payload) { ByteBuf compressedPayload = compressor.encode(payload); payload.release(); return compressedPayload; @@ -540,22 +541,29 @@ public void sendAsync(Message message, SendCallback callback) { boolean compressed = false; // Batch will be compressed when closed // If a message has a delayed delivery time, we'll always send it individually - if (!isBatchMessagingEnabled() || msgMetadata.hasDeliverAtTime()) { - compressedPayload = applyCompression(payload); - compressed = true; + if (((!isBatchMessagingEnabled() || msgMetadata.hasDeliverAtTime()))) { + if (payload.readableBytes() < conf.getCompressMinMsgBodySize()) { - // validate msg-size (For batching this will be check at the batch completion size) - int compressedSize = compressedPayload.readableBytes(); - if (compressedSize > getMaxMessageSize() && !this.conf.isChunkingEnabled()) { - compressedPayload.release(); - String compressedStr = conf.getCompressionType() != CompressionType.NONE ? "Compressed" : ""; - PulsarClientException.InvalidMessageException invalidMessageException = - new PulsarClientException.InvalidMessageException( - format("The producer %s of the topic %s sends a %s message with %d bytes that exceeds" - + " %d bytes", - producerName, topic, compressedStr, compressedSize, getMaxMessageSize())); - completeCallbackAndReleaseSemaphore(uncompressedSize, callback, invalidMessageException); - return; + } else { + compressedPayload = applyCompression(payload); + compressed = true; + + // validate msg-size (For batching this will be check at the batch completion size) + int compressedSize = compressedPayload.readableBytes(); + if (compressedSize > getMaxMessageSize() && !this.conf.isChunkingEnabled()) { + compressedPayload.release(); + String compressedStr = conf.getCompressionType() != CompressionType.NONE + ? ("compressed (" + conf.getCompressionType() + ")") + : "uncompressed"; + PulsarClientException.InvalidMessageException invalidMessageException = + new PulsarClientException.InvalidMessageException( + format("The producer %s of the topic %s sends a %s message with %d bytes that " + + "exceeds %d bytes", + producerName, topic, compressedStr, compressedSize, + getMaxMessageSize())); + completeCallbackAndReleaseSemaphore(uncompressedSize, callback, invalidMessageException); + return; + } } } @@ -577,7 +585,7 @@ public void sendAsync(Message message, SendCallback callback) { // Update the message metadata before computing the payload chunk size to avoid a large message cannot be split // into chunks. - updateMessageMetadata(msgMetadata, uncompressedSize); + updateMessageMetadata(msgMetadata, uncompressedSize, compressed); // send in chunks int totalChunks; @@ -673,7 +681,9 @@ public void sendAsync(Message message, SendCallback callback) { * @param uncompressedSize * @return the sequence id */ - private void updateMessageMetadata(final MessageMetadata msgMetadata, final int uncompressedSize) { + @SuppressWarnings("checkstyle:Indentation") + private void updateMessageMetadata(final MessageMetadata msgMetadata, final int uncompressedSize, + boolean isCompressed) { if (!msgMetadata.hasPublishTime()) { msgMetadata.setPublishTime(client.getClientClock().millis()); @@ -683,9 +693,9 @@ private void updateMessageMetadata(final MessageMetadata msgMetadata, final int // The field "uncompressedSize" is zero means the compression info were not set yet. if (msgMetadata.getUncompressedSize() <= 0) { - if (conf.getCompressionType() != CompressionType.NONE) { - msgMetadata - .setCompression(CompressionCodecProvider.convertToWireProtocol(conf.getCompressionType())); + if (conf.getCompressionType() != CompressionType.NONE && isCompressed) { + msgMetadata.setCompression( + CompressionCodecProvider.convertToWireProtocol(conf.getCompressionType())); } msgMetadata.setUncompressedSize(uncompressedSize); } @@ -777,7 +787,7 @@ private void serializeAndSendMessage(MessageImpl msg, } else { // in this case compression has not been applied by the caller // but we have to compress the payload if compression is configured - if (!compressed) { + if (!compressed && chunkPayload.readableBytes() > conf.getCompressMinMsgBodySize()) { chunkPayload = applyCompression(chunkPayload); } ByteBuf encryptedPayload = encryptMessage(msgMetadata, chunkPayload); diff --git a/pulsar-client/src/main/java/org/apache/pulsar/client/impl/conf/ProducerConfigurationData.java b/pulsar-client/src/main/java/org/apache/pulsar/client/impl/conf/ProducerConfigurationData.java index 6ec738bbf4c8d..0c770c7c9bd05 100644 --- a/pulsar-client/src/main/java/org/apache/pulsar/client/impl/conf/ProducerConfigurationData.java +++ b/pulsar-client/src/main/java/org/apache/pulsar/client/impl/conf/ProducerConfigurationData.java @@ -189,6 +189,8 @@ public class ProducerConfigurationData implements Serializable, Cloneable { ) private CompressionType compressionType = CompressionType.NONE; + private int compressMinMsgBodySize = 4 * 1024; // 4kb + // Cannot use Optional since it's not serializable private Long initialSequenceId = null; From 40b96de0410c19551d1700501e28d8a8c4a63336 Mon Sep 17 00:00:00 2001 From: Zixuan Liu Date: Mon, 17 Feb 2025 14:46:45 +0800 Subject: [PATCH 12/16] [fix][build] Add develops for buildtools (#23992) Signed-off-by: Zixuan Liu --- buildtools/pom.xml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/buildtools/pom.xml b/buildtools/pom.xml index 6846cf01e4965..7c78924a371e2 100644 --- a/buildtools/pom.xml +++ b/buildtools/pom.xml @@ -35,6 +35,13 @@ jar Pulsar Build Tools + + + Apache Pulsar developers + http://pulsar.apache.org/ + + + 2024-10-14T13:32:50Z 1.8 From 5c5b44b4b238dabba38dfcd001e3f716d9a90fa4 Mon Sep 17 00:00:00 2001 From: Yunze Xu Date: Mon, 17 Feb 2025 23:48:48 +0800 Subject: [PATCH 13/16] [revert] "[improve][ml] Do not switch thread to execute asyncAddEntry's core logic (#23940)" (#23994) --- .../mledger/impl/ManagedLedgerImpl.java | 52 ++++++++---------- .../mledger/impl/ShadowManagedLedgerImpl.java | 16 +++--- .../ManagedLedgerInterceptorImplTest.java | 53 ------------------- 3 files changed, 31 insertions(+), 90 deletions(-) diff --git a/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java b/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java index 7426059e576f6..607b6d09cc239 100644 --- a/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java +++ b/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java @@ -802,41 +802,33 @@ public void asyncAddEntry(ByteBuf buffer, int numberOfMessages, AddEntryCallback buffer.retain(); // Jump to specific thread to avoid contention from writers writing from different threads - final var addOperation = OpAddEntry.createNoRetainBuffer(this, buffer, numberOfMessages, callback, ctx, - currentLedgerTimeoutTriggered); - var added = false; - try { - // Use synchronized to ensure if `addOperation` is added to queue and fails later, it will be the first - // element in `pendingAddEntries`. - synchronized (this) { - if (managedLedgerInterceptor != null) { - managedLedgerInterceptor.beforeAddEntry(addOperation, addOperation.getNumberOfMessages()); - } - final var state = STATE_UPDATER.get(this); - beforeAddEntryToQueue(state); - pendingAddEntries.add(addOperation); - added = true; - afterAddEntryToQueue(state, addOperation); - } - } catch (Throwable throwable) { - if (!added) { - addOperation.failed(ManagedLedgerException.getManagedLedgerException(throwable)); - } // else: all elements of `pendingAddEntries` will fail in another thread - } + executor.execute(() -> { + OpAddEntry addOperation = OpAddEntry.createNoRetainBuffer(this, buffer, numberOfMessages, callback, ctx, + currentLedgerTimeoutTriggered); + internalAsyncAddEntry(addOperation); + }); } - protected void beforeAddEntryToQueue(State state) throws ManagedLedgerException { - if (state.isFenced()) { - throw new ManagedLedgerFencedException(); + protected synchronized void internalAsyncAddEntry(OpAddEntry addOperation) { + if (!beforeAddEntry(addOperation)) { + return; } - switch (state) { - case Terminated -> throw new ManagedLedgerTerminatedException("Managed ledger was already terminated"); - case Closed -> throw new ManagedLedgerAlreadyClosedException("Managed ledger was already closed"); - case WriteFailed -> throw new ManagedLedgerAlreadyClosedException("Waiting to recover from failure"); + final State state = STATE_UPDATER.get(this); + if (state.isFenced()) { + addOperation.failed(new ManagedLedgerFencedException()); + return; + } else if (state == State.Terminated) { + addOperation.failed(new ManagedLedgerTerminatedException("Managed ledger was already terminated")); + return; + } else if (state == State.Closed) { + addOperation.failed(new ManagedLedgerAlreadyClosedException("Managed ledger was already closed")); + return; + } else if (state == State.WriteFailed) { + addOperation.failed(new ManagedLedgerAlreadyClosedException("Waiting to recover from failure")); + return; } - } + pendingAddEntries.add(addOperation); - protected void afterAddEntryToQueue(State state, OpAddEntry addOperation) throws ManagedLedgerException { if (state == State.ClosingLedger || state == State.CreatingLedger) { // We don't have a ready ledger to write into // We are waiting for a new ledger to be created diff --git a/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ShadowManagedLedgerImpl.java b/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ShadowManagedLedgerImpl.java index bae6cd66d2825..4b03cad8e0a1d 100644 --- a/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ShadowManagedLedgerImpl.java +++ b/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ShadowManagedLedgerImpl.java @@ -223,23 +223,25 @@ private void initLastConfirmedEntry() { } @Override - protected void beforeAddEntryToQueue(State state) throws ManagedLedgerException { + protected synchronized void internalAsyncAddEntry(OpAddEntry addOperation) { + if (!beforeAddEntry(addOperation)) { + return; + } if (state != State.LedgerOpened) { - throw new ManagedLedgerException("Managed ledger is not opened"); + addOperation.failed(new ManagedLedgerException("Managed ledger is not opened")); + return; } - } - @Override - protected void afterAddEntryToQueue(State state, OpAddEntry addOperation) throws ManagedLedgerException { if (addOperation.getCtx() == null || !(addOperation.getCtx() instanceof Position position)) { - pendingAddEntries.poll(); - throw new ManagedLedgerException("Illegal addOperation context object."); + addOperation.failed(new ManagedLedgerException("Illegal addOperation context object.")); + return; } if (log.isDebugEnabled()) { log.debug("[{}] Add entry into shadow ledger lh={} entries={}, pos=({},{})", name, currentLedger.getId(), currentLedgerEntries, position.getLedgerId(), position.getEntryId()); } + pendingAddEntries.add(addOperation); if (position.getLedgerId() <= currentLedger.getId()) { // Write into lastLedger if (position.getLedgerId() == currentLedger.getId()) { diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/intercept/ManagedLedgerInterceptorImplTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/intercept/ManagedLedgerInterceptorImplTest.java index 8663019efb8c1..b57b5ce94be42 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/intercept/ManagedLedgerInterceptorImplTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/intercept/ManagedLedgerInterceptorImplTest.java @@ -21,7 +21,6 @@ import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertNotEquals; import static org.testng.Assert.assertNotNull; -import static org.testng.Assert.assertTrue; import io.netty.buffer.ByteBuf; import io.netty.buffer.Unpooled; @@ -30,12 +29,9 @@ import java.util.Collections; import java.util.HashSet; import java.util.List; -import java.util.Random; import java.util.Set; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CountDownLatch; -import java.util.concurrent.Executors; -import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Predicate; import lombok.Cleanup; @@ -503,53 +499,4 @@ public void addFailed(ManagedLedgerException exception, Object ctx) { ledger.close(); } - @Test - public void testBeforeAddEntry() throws Exception { - final var interceptor = new ManagedLedgerInterceptorImpl(getBrokerEntryMetadataInterceptors(), null); - final var config = new ManagedLedgerConfig(); - final var numEntries = 100; - config.setMaxEntriesPerLedger(numEntries); - config.setManagedLedgerInterceptor(interceptor); - @Cleanup final var ml = (ManagedLedgerImpl) factory.open("test_concurrent_add_entry", config); - - final var indexesBeforeAdd = new ArrayList(); - final var batchSizes = new ArrayList(); - final var random = new Random(); - final var latch = new CountDownLatch(numEntries); - final var executor = Executors.newFixedThreadPool(3); - final var lock = new Object(); // make sure `asyncAddEntry` are called in order - for (int i = 0; i < numEntries; i++) { - final var batchSize = random.nextInt(0, 100); - final var msg = "msg-" + i; - final var callback = new AsyncCallbacks.AddEntryCallback() { - - @Override - public void addComplete(Position position, ByteBuf entryData, Object ctx) { - latch.countDown(); - } - - @Override - public void addFailed(ManagedLedgerException exception, Object ctx) { - log.error("Failed to add {}", msg, exception); - latch.countDown(); - } - }; - executor.execute(() -> { - synchronized (lock) { - batchSizes.add((long) batchSize); - indexesBeforeAdd.add(interceptor.getIndex() + 1); // index is updated in each asyncAddEntry call - ml.asyncAddEntry(Unpooled.wrappedBuffer(msg.getBytes()), batchSize, callback, null); - } - }); - } - assertTrue(latch.await(3, TimeUnit.SECONDS)); - synchronized (lock) { - for (int i = 1; i < numEntries; i++) { - final var sum = batchSizes.get(i) + batchSizes.get(i - 1); - batchSizes.set(i, sum); - } - assertEquals(indexesBeforeAdd.subList(1, numEntries), batchSizes.subList(0, numEntries - 1)); - } - executor.shutdown(); - } } From df5197212e8806c7d1907dedfcdfd9e40a4f0ea5 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Mon, 17 Feb 2025 18:54:43 +0200 Subject: [PATCH 14/16] [fix][meta] Fix ephemeral handling of ZK nodes and fix MockZooKeeper ephemeral and ZK stat handling (#23988) --- .../loadbalance/impl/LoadManagerShared.java | 4 +- .../impl/ModularLoadManagerImpl.java | 2 +- .../pulsar/broker/web/PulsarWebResource.java | 2 +- .../common/naming/NamespaceBundles.java | 8 +- .../auth/MockedPulsarServiceBaseTest.java | 27 +- .../broker/testcontext/PulsarTestContext.java | 93 +- .../client/api/BrokerServiceLookupTest.java | 113 +- .../client/api/ProducerConsumerBase.java | 2 +- .../pulsar/metadata/impl/ZKMetadataStore.java | 14 +- .../BookKeeperClusterTestCase.java | 4 + .../metadata/BaseMetadataStoreTest.java | 128 +- .../apache/pulsar/metadata/CounterTest.java | 3 +- .../pulsar/metadata/MetadataCacheTest.java | 29 +- .../metadata/MetadataStoreExtendedTest.java | 6 +- .../pulsar/metadata/MetadataStoreTest.java | 27 +- .../MockZooKeeperMetadataStoreProvider.java | 49 + .../bookkeeper/LedgerManagerIteratorTest.java | 18 +- .../LedgerUnderreplicationManagerTest.java | 8 +- .../PulsarLedgerIdGeneratorTest.java | 2 +- .../impl/MetadataStoreFactoryImplTest.java | 21 +- .../org/apache/zookeeper/MockZooKeeper.java | 1199 +++++++++-------- .../zookeeper/MockZooKeeperSession.java | 201 ++- 22 files changed, 1199 insertions(+), 761 deletions(-) create mode 100644 pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MockZooKeeperMetadataStoreProvider.java diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/LoadManagerShared.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/LoadManagerShared.java index 7ca2b926db7db..591b061253d3b 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/LoadManagerShared.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/LoadManagerShared.java @@ -285,7 +285,7 @@ public static CompletableFuture> applyNamespacePoliciesAsync( public static String getBundleRangeFromBundleName(String bundleName) { // the bundle format is property/cluster/namespace/0x00000000_0xFFFFFFFF int pos = bundleName.lastIndexOf("/"); - checkArgument(pos != -1); + checkArgument(pos != -1, "Invalid bundle name format: %s", bundleName); return bundleName.substring(pos + 1); } @@ -293,7 +293,7 @@ public static String getBundleRangeFromBundleName(String bundleName) { public static String getNamespaceNameFromBundleName(String bundleName) { // the bundle format is property/cluster/namespace/0x00000000_0xFFFFFFFF int pos = bundleName.lastIndexOf('/'); - checkArgument(pos != -1); + checkArgument(pos != -1, "Invalid bundle name format: %s", bundleName); return bundleName.substring(0, pos); } diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ModularLoadManagerImpl.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ModularLoadManagerImpl.java index 48a6121b9dd13..f86b608d93722 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ModularLoadManagerImpl.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ModularLoadManagerImpl.java @@ -583,7 +583,7 @@ private void updateBundleData() { for (String bundle : bundleData.keySet()) { if (!activeBundles.contains(bundle)){ bundleData.remove(bundle); - if (pulsar.getLeaderElectionService().isLeader()){ + if (pulsar.getLeaderElectionService() != null && pulsar.getLeaderElectionService().isLeader()){ deleteBundleDataFromMetadataStore(bundle); } } diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java index 2e198eb99752e..f191fc3a38423 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java @@ -1012,7 +1012,7 @@ protected static boolean isLeaderBroker(PulsarService pulsar) { if (ExtensibleLoadManagerImpl.isLoadManagerExtensionEnabled(pulsar)) { return true; } - return pulsar.getLeaderElectionService().isLeader(); + return pulsar.getLeaderElectionService() != null && pulsar.getLeaderElectionService().isLeader(); } public void validateTenantOperation(String tenant, TenantOperation operation) { diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/common/naming/NamespaceBundles.java b/pulsar-broker/src/main/java/org/apache/pulsar/common/naming/NamespaceBundles.java index 3ee365cdd4571..27c73edc6b597 100644 --- a/pulsar-broker/src/main/java/org/apache/pulsar/common/naming/NamespaceBundles.java +++ b/pulsar-broker/src/main/java/org/apache/pulsar/common/naming/NamespaceBundles.java @@ -108,9 +108,11 @@ public int size() { public void validateBundle(NamespaceBundle nsBundle) throws Exception { int idx = Arrays.binarySearch(partitions, nsBundle.getLowerEndpoint()); - checkArgument(idx >= 0, "Cannot find bundle in the bundles list"); - checkArgument(nsBundle.getUpperEndpoint().equals(bundles.get(idx).getUpperEndpoint()), - "Invalid upper boundary for bundle"); + checkArgument(idx >= 0, "Cannot find bundle %s in the bundles list", nsBundle); + NamespaceBundle foundBundle = bundles.get(idx); + Long upperEndpoint = foundBundle.getUpperEndpoint(); + checkArgument(nsBundle.getUpperEndpoint().equals(upperEndpoint), + "Invalid upper boundary for bundle %s. Expected upper boundary of %s", nsBundle, foundBundle); } public NamespaceBundle getFullBundle() { diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/MockedPulsarServiceBaseTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/MockedPulsarServiceBaseTest.java index 42e2c00f73acf..5be5c7544524f 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/MockedPulsarServiceBaseTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/MockedPulsarServiceBaseTest.java @@ -150,6 +150,9 @@ public static String getTlsFileForClient(String name) { private final List closeables = new ArrayList<>(); + // Set to true in test's constructor to use a real Zookeeper (TestZKServer) + protected boolean useTestZookeeper; + public MockedPulsarServiceBaseTest() { resetConfig(); } @@ -363,7 +366,14 @@ protected void afterPulsarStart(PulsarService pulsar) throws Exception { * @throws Exception if an error occurs */ protected void restartBroker() throws Exception { + restartBroker(null); + } + + protected void restartBroker(Consumer configurationChanger) throws Exception { stopBroker(); + if (configurationChanger != null) { + configurationChanger.accept(conf); + } startBroker(); if (pulsarClient == null) { pulsarClient = newPulsarClient(lookupUrl.toString(), 0); @@ -461,7 +471,6 @@ protected PulsarTestContext.Builder createPulsarTestContextBuilder(ServiceConfig PulsarTestContext.Builder builder = PulsarTestContext.builder() .spyByDefault() .config(conf) - .withMockZookeeper(true) .pulsarServiceCustomizer(pulsarService -> { try { beforePulsarStart(pulsarService); @@ -470,9 +479,25 @@ protected PulsarTestContext.Builder createPulsarTestContextBuilder(ServiceConfig } }) .brokerServiceCustomizer(this::customizeNewBrokerService); + configureMetadataStores(builder); return builder; } + /** + * Configures the metadata stores for the PulsarTestContext.Builder instance. + * Set useTestZookeeper to true in the test's constructor to use TestZKServer which is a real ZooKeeper + * implementation. + * + * @param builder the PulsarTestContext.Builder instance to configure + */ + protected void configureMetadataStores(PulsarTestContext.Builder builder) { + if (useTestZookeeper) { + builder.withTestZookeeper(); + } else { + builder.withMockZookeeper(true); + } + } + protected PulsarTestContext createAdditionalPulsarTestContext(ServiceConfiguration conf) throws Exception { return createAdditionalPulsarTestContext(conf, null); } diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/broker/testcontext/PulsarTestContext.java b/pulsar-broker/src/test/java/org/apache/pulsar/broker/testcontext/PulsarTestContext.java index 6403c3bcec4c3..f8205a2e939a3 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/broker/testcontext/PulsarTestContext.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/broker/testcontext/PulsarTestContext.java @@ -19,7 +19,6 @@ package org.apache.pulsar.broker.testcontext; -import com.google.common.util.concurrent.MoreExecutors; import io.netty.channel.EventLoopGroup; import io.opentelemetry.api.OpenTelemetry; import io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdkBuilder; @@ -27,7 +26,6 @@ import java.io.IOException; import java.nio.charset.StandardCharsets; import java.time.Duration; -import java.util.ArrayList; import java.util.Collection; import java.util.List; import java.util.Optional; @@ -37,6 +35,7 @@ import lombok.Builder; import lombok.Getter; import lombok.Singular; +import lombok.SneakyThrows; import lombok.ToString; import lombok.extern.slf4j.Slf4j; import org.apache.bookkeeper.client.BookKeeper; @@ -65,6 +64,7 @@ import org.apache.pulsar.compaction.CompactionServiceFactory; import org.apache.pulsar.compaction.Compactor; import org.apache.pulsar.compaction.PulsarCompactionServiceFactory; +import org.apache.pulsar.metadata.TestZKServer; import org.apache.pulsar.metadata.api.MetadataStore; import org.apache.pulsar.metadata.api.MetadataStoreConfig; import org.apache.pulsar.metadata.api.MetadataStoreException; @@ -72,9 +72,11 @@ import org.apache.pulsar.metadata.impl.MetadataStoreFactoryImpl; import org.apache.pulsar.metadata.impl.ZKMetadataStore; import org.apache.zookeeper.CreateMode; +import org.apache.zookeeper.KeeperException; import org.apache.zookeeper.MockZooKeeper; import org.apache.zookeeper.MockZooKeeperSession; -import org.apache.zookeeper.data.ACL; +import org.apache.zookeeper.ZooDefs; +import org.apache.zookeeper.ZooKeeper; import org.jetbrains.annotations.NotNull; import org.mockito.Mockito; import org.mockito.internal.util.MockUtil; @@ -161,6 +163,10 @@ public class PulsarTestContext implements AutoCloseable { private final MockZooKeeper mockZooKeeperGlobal; + private final TestZKServer testZKServer; + + private final TestZKServer testZKServerGlobal; + private final SpyConfig spyConfig; private final boolean startable; @@ -409,6 +415,11 @@ public Builder reuseMockBookkeeperAndMetadataStores(PulsarTestContext otherConte if (otherContext.getMockZooKeeperGlobal() != null) { mockZooKeeperGlobal(otherContext.getMockZooKeeperGlobal()); } + } else if (otherContext.getTestZKServer() != null) { + testZKServer(otherContext.getTestZKServer()); + if (otherContext.getTestZKServerGlobal() != null) { + testZKServerGlobal(otherContext.getTestZKServerGlobal()); + } } else { localMetadataStore(NonClosingProxyHandler.createNonClosingProxy(otherContext.getLocalMetadataStore(), MetadataStoreExtended.class @@ -476,17 +487,56 @@ public Builder withMockZookeeper(boolean useSeparateGlobalZk) { } private MockZooKeeper createMockZooKeeper() throws Exception { - MockZooKeeper zk = MockZooKeeper.newInstance(MoreExecutors.newDirectExecutorService()); - List dummyAclList = new ArrayList<>(0); + MockZooKeeper zk = MockZooKeeper.newInstance(); + initializeZookeeper(zk); + registerCloseable(zk::shutdown); + return zk; + } + private static void initializeZookeeper(ZooKeeper zk) throws KeeperException, InterruptedException { ZkUtils.createFullPathOptimistic(zk, "/ledgers/available/192.168.1.1:" + 5000, - "".getBytes(StandardCharsets.UTF_8), dummyAclList, CreateMode.PERSISTENT); + "".getBytes(StandardCharsets.UTF_8), ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT); - zk.create("/ledgers/LAYOUT", "1\nflat:1".getBytes(StandardCharsets.UTF_8), dummyAclList, + zk.create("/ledgers/LAYOUT", "1\nflat:1".getBytes(StandardCharsets.UTF_8), ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT); + } - registerCloseable(zk::shutdown); - return zk; + /** + * Configure this PulsarTestContext to use a test ZooKeeper instance which is + * shared for both the local and configuration metadata stores. + * + * @return the builder + */ + public Builder withTestZookeeper() { + return withTestZookeeper(false); + } + + /** + * Configure this PulsarTestContext to use a test ZooKeeper instance. + * + * @param useSeparateGlobalZk if true, the global (configuration) zookeeper will be a separate instance + * @return the builder + */ + public Builder withTestZookeeper(boolean useSeparateGlobalZk) { + try { + testZKServer(createTestZookeeper()); + if (useSeparateGlobalZk) { + testZKServerGlobal(createTestZookeeper()); + } + } catch (Exception e) { + throw new RuntimeException(e); + } + return this; + } + + private TestZKServer createTestZookeeper() throws Exception { + TestZKServer testZKServer = new TestZKServer(); + try (ZooKeeper zkc = new ZooKeeper(testZKServer.getConnectionString(), 5000, event -> { + })) { + initializeZookeeper(zkc); + } + registerCloseable(testZKServer); + return testZKServer; } /** @@ -676,6 +726,20 @@ private void initializeCommonPulsarServices(SpyConfig spyConfig) { configurationMetadataStore(mockZookeeperMetadataStore); } } + } else if (super.testZKServer != null) { + MetadataStoreExtended testZookeeperMetadataStore = + createTestZookeeperMetadataStore(super.testZKServer, MetadataStoreConfig.METADATA_STORE); + if (super.localMetadataStore == null) { + localMetadataStore(testZookeeperMetadataStore); + } + if (super.configurationMetadataStore == null) { + if (super.testZKServerGlobal != null) { + configurationMetadataStore(createTestZookeeperMetadataStore(super.testZKServerGlobal, + MetadataStoreConfig.CONFIGURATION_METADATA_STORE)); + } else { + configurationMetadataStore(testZookeeperMetadataStore); + } + } } else { try { MetadataStoreExtended store = MetadataStoreFactoryImpl.createExtended("memory:local", @@ -720,6 +784,17 @@ private MetadataStoreExtended createMockZookeeperMetadataStore(MockZooKeeper moc return nonClosingProxy; } + @SneakyThrows + private MetadataStoreExtended createTestZookeeperMetadataStore(TestZKServer zkServer, + String metadataStoreName) { + MetadataStoreExtended store = MetadataStoreExtended.create("zk:" + zkServer.getConnectionString(), + MetadataStoreConfig.builder().metadataStoreName(metadataStoreName).build()); + registerCloseable(store); + MetadataStoreExtended nonClosingProxy = + NonClosingProxyHandler.createNonClosingProxy(store, MetadataStoreExtended.class); + return nonClosingProxy; + } + protected abstract void initializePulsarServices(SpyConfig spyConfig, Builder builder); } diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/client/api/BrokerServiceLookupTest.java b/pulsar-broker/src/test/java/org/apache/pulsar/client/api/BrokerServiceLookupTest.java index 07deb9007c487..d0cb4266ae10a 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/client/api/BrokerServiceLookupTest.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/client/api/BrokerServiceLookupTest.java @@ -92,6 +92,7 @@ import org.apache.pulsar.common.naming.TopicName; import org.apache.pulsar.common.partition.PartitionedTopicMetadata; import org.apache.pulsar.common.policies.data.ClusterData; +import org.apache.pulsar.common.policies.data.PoliciesUtil; import org.apache.pulsar.common.policies.data.TenantInfoImpl; import org.apache.pulsar.common.util.ObjectMapperFactory; import org.apache.pulsar.common.util.SecurityUtility; @@ -112,15 +113,41 @@ import org.awaitility.reflect.WhiteboxImpl; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.testng.ITest; +import org.testng.SkipException; import org.testng.annotations.AfterMethod; import org.testng.annotations.BeforeMethod; +import org.testng.annotations.DataProvider; +import org.testng.annotations.Factory; import org.testng.annotations.Test; @Test(groups = "broker-api") -public class BrokerServiceLookupTest extends ProducerConsumerBase { +public class BrokerServiceLookupTest extends ProducerConsumerBase implements ITest { private static final Logger log = LoggerFactory.getLogger(BrokerServiceLookupTest.class); + private String testName; + + @DataProvider + private static Object[] booleanValues() { + return new Object[]{ true, false }; + } + + @Factory(dataProvider = "booleanValues") + public BrokerServiceLookupTest(boolean useTestZookeeper) { + // when set to true, TestZKServer is used which is a real ZooKeeper implementation + this.useTestZookeeper = useTestZookeeper; + } + + @Override + public String getTestName() { + return testName; + } @BeforeMethod + public void applyTestName(Method method) { + testName = method.getName() + " with " + (useTestZookeeper ? "TestZKServer" : "MockZooKeeper"); + } + + @BeforeMethod(dependsOnMethods = "setTestMethodName") @Override protected void setup() throws Exception { conf.setDefaultNumberOfNamespaceBundles(1); @@ -129,10 +156,43 @@ protected void setup() throws Exception { producerBaseSetup(); } + @Override + protected void doInitConf() throws Exception { + super.doInitConf(); + switch (methodName) { + case "testMultipleBrokerDifferentClusterLookup" -> { + conf.setAuthenticationEnabled(true); + } + case "testWebserviceServiceTls" -> { + // broker1 with tls enabled + conf.setBrokerServicePortTls(Optional.of(0)); + conf.setWebServicePortTls(Optional.of(0)); + conf.setTlsTrustCertsFilePath(CA_CERT_FILE_PATH); + conf.setTlsRequireTrustedClientCertOnConnect(true); + conf.setTlsCertificateFilePath(BROKER_CERT_FILE_PATH); + conf.setTlsKeyFilePath(BROKER_KEY_FILE_PATH); + conf.setNumExecutorThreadPoolSize(5); + // Not in use, and because TLS is not configured, it will fail to start + conf.setSystemTopicEnabled(false); + } + case "testSkipSplitBundleIfOnlyOneBroker" -> { + conf.setDefaultNumberOfNamespaceBundles(1); + conf.setLoadBalancerNamespaceBundleMaxTopics(1); + conf.setLoadManagerClassName(ModularLoadManagerImpl.class.getName()); + } + case "testPartitionedMetadataWithDeprecatedVersion" -> { + conf.setBrokerServicePortTls(Optional.empty()); + conf.setWebServicePortTls(Optional.empty()); + conf.setClientLibraryVersionCheckEnabled(true); + } + } + } + @AfterMethod(alwaysRun = true) @Override protected void cleanup() throws Exception { internalCleanup(); + testName = null; } @Override @@ -295,9 +355,11 @@ public void testMultipleBrokerLookup() throws Exception { @Test public void testConcurrentWriteBrokerData() throws Exception { Map map = new ConcurrentHashMap<>(); + List boundaries = PoliciesUtil.getBundles(100).getBoundaries(); for (int i = 0; i < 100; i++) { - map.put("key"+ i, new NamespaceBundleStats()); + map.put("my-property/my-ns/" + boundaries.get(i), new NamespaceBundleStats()); } + BrokerService originalBrokerService = pulsar.getBrokerService(); BrokerService brokerService = mock(BrokerService.class); doReturn(brokerService).when(pulsar).getBrokerService(); doReturn(map).when(brokerService).getBundleStats(); @@ -328,6 +390,8 @@ public void testConcurrentWriteBrokerData() throws Exception { for (Future future : list) { future.get(); } + // allow proper shutdown so that resources aren't leaked + doReturn(originalBrokerService).when(pulsar).getBrokerService(); } /** @@ -375,12 +439,6 @@ public void testMultipleBrokerDifferentClusterLookup() throws Exception { @Cleanup PulsarClient pulsarClient2 = PulsarClient.builder().serviceUrl(brokerServiceUrl.toString()).build(); - // enable authorization: so, broker can validate cluster and redirect if finds different cluster - pulsar.getConfiguration().setAuthorizationEnabled(true); - // restart broker with authorization enabled: it initialize AuthorizationService - stopBroker(); - startBroker(); - LoadManager loadManager2 = spy(pulsar2.getLoadManager().get()); Field loadManagerField = NamespaceService.class.getDeclaredField("loadManager"); loadManagerField.setAccessible(true); @@ -417,10 +475,6 @@ public void testMultipleBrokerDifferentClusterLookup() throws Exception { consumer.acknowledgeCumulative(msg); consumer.close(); producer.close(); - - // disable authorization - pulsar.getConfiguration().setAuthorizationEnabled(false); - loadManager2 = null; } /** @@ -538,18 +592,6 @@ public void testWebserviceServiceTls() throws Exception { PulsarTestContext pulsarTestContext2 = createAdditionalPulsarTestContext(conf2); PulsarService pulsar2 = pulsarTestContext2.getPulsarService(); - // restart broker1 with tls enabled - conf.setBrokerServicePortTls(Optional.of(0)); - conf.setWebServicePortTls(Optional.of(0)); - conf.setTlsTrustCertsFilePath(CA_CERT_FILE_PATH); - conf.setTlsRequireTrustedClientCertOnConnect(true); - conf.setTlsCertificateFilePath(BROKER_CERT_FILE_PATH); - conf.setTlsKeyFilePath(BROKER_KEY_FILE_PATH); - conf.setNumExecutorThreadPoolSize(5); - // Not in use, and because TLS is not configured, it will fail to start - conf.setSystemTopicEnabled(false); - stopBroker(); - startBroker(); pulsar.getLoadManager().get().writeLoadReportOnZookeeper(); pulsar2.getLoadManager().get().writeLoadReportOnZookeeper(); @@ -753,11 +795,6 @@ public void testModularLoadManagerSplitBundle() throws Exception { conf2.setLoadBalancerAutoUnloadSplitBundlesEnabled(true); conf2.setLoadBalancerNamespaceBundleMaxTopics(1); - // configure broker-1 with ModularLoadManager - stopBroker(); - conf.setLoadManagerClassName(ModularLoadManagerImpl.class.getName()); - startBroker(); - @Cleanup PulsarTestContext pulsarTestContext2 = createAdditionalPulsarTestContext(conf2); PulsarService pulsar2 = pulsarTestContext2.getPulsarService(); @@ -875,12 +912,6 @@ public void testSkipSplitBundleIfOnlyOneBroker() throws Exception { final String topicName1 = BrokerTestUtil.newUniqueName("persistent://" + namespace + "/tp_"); final String topicName2 = BrokerTestUtil.newUniqueName("persistent://" + namespace + "/tp_"); try { - // configure broker with ModularLoadManager. - stopBroker(); - conf.setDefaultNumberOfNamespaceBundles(1); - conf.setLoadBalancerNamespaceBundleMaxTopics(1); - conf.setLoadManagerClassName(ModularLoadManagerImpl.class.getName()); - startBroker(); final ModularLoadManagerWrapper modularLoadManagerWrapper = (ModularLoadManagerWrapper) pulsar.getLoadManager().get(); final ModularLoadManagerImpl modularLoadManager = @@ -1033,12 +1064,6 @@ public void testPartitionedMetadataWithDeprecatedVersion() throws Exception { admin.namespaces().createNamespace(property + "/" + cluster + "/" + namespace); admin.topics().createPartitionedTopic(dest.toString(), totalPartitions); - stopBroker(); - conf.setBrokerServicePortTls(Optional.empty()); - conf.setWebServicePortTls(Optional.empty()); - conf.setClientLibraryVersionCheckEnabled(true); - startBroker(); - URI brokerServiceUrl = new URI(pulsar.getSafeWebServiceAddress()); URL url = brokerServiceUrl.toURL(); @@ -1197,6 +1222,9 @@ public String authenticate(AuthenticationDataSource authData) throws Authenticat @Test public void testLookupConnectionNotCloseIfGetUnloadingExOrMetadataEx() throws Exception { + if (useTestZookeeper) { + throw new SkipException("This test case depends on MockZooKeeper"); + } String tpName = BrokerTestUtil.newUniqueName("persistent://public/default/tp"); admin.topics().createNonPartitionedTopic(tpName); PulsarClientImpl pulsarClientImpl = (PulsarClientImpl) pulsarClient; @@ -1312,7 +1340,8 @@ private void makeAcquireBundleLockSuccess() throws Exception { } } - @Test(timeOut = 30000) + // TODO: This test is disabled since it's invalid. The test fails for both TestZKServer and MockZooKeeper. + @Test(timeOut = 30000, enabled = false) public void testLookupConnectionNotCloseIfFailedToAcquireOwnershipOfBundle() throws Exception { String tpName = BrokerTestUtil.newUniqueName("persistent://public/default/tp"); admin.topics().createNonPartitionedTopic(tpName); diff --git a/pulsar-broker/src/test/java/org/apache/pulsar/client/api/ProducerConsumerBase.java b/pulsar-broker/src/test/java/org/apache/pulsar/client/api/ProducerConsumerBase.java index 0cf2e49d35bee..01d2f107dffe0 100644 --- a/pulsar-broker/src/test/java/org/apache/pulsar/client/api/ProducerConsumerBase.java +++ b/pulsar-broker/src/test/java/org/apache/pulsar/client/api/ProducerConsumerBase.java @@ -40,7 +40,7 @@ public abstract class ProducerConsumerBase extends MockedPulsarServiceBaseTest { protected String methodName; @BeforeMethod(alwaysRun = true) - public void beforeMethod(Method m) throws Exception { + public void setTestMethodName(Method m) throws Exception { methodName = m.getName(); } diff --git a/pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/ZKMetadataStore.java b/pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/ZKMetadataStore.java index 8fd8252152898..dab3553f8ce1b 100644 --- a/pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/ZKMetadataStore.java +++ b/pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/ZKMetadataStore.java @@ -71,9 +71,10 @@ @Slf4j public class ZKMetadataStore extends AbstractBatchedMetadataStore implements MetadataStoreExtended, MetadataStoreLifecycle { - public static final String ZK_SCHEME = "zk"; public static final String ZK_SCHEME_IDENTIFIER = "zk:"; + // ephemeralOwner value for persistent nodes + private static final long NOT_EPHEMERAL = 0L; private final String zkConnectString; private final String rootPath; @@ -129,12 +130,17 @@ public ZKMetadataStore(ZooKeeper zkc) { @VisibleForTesting @SneakyThrows public ZKMetadataStore(ZooKeeper zkc, MetadataStoreConfig config) { - super(config); + this(zkc, config, false); + } + @VisibleForTesting + @SneakyThrows + public ZKMetadataStore(ZooKeeper zkc, MetadataStoreConfig config, boolean isZkManaged) { + super(config); this.zkConnectString = null; this.rootPath = null; this.metadataStoreConfig = null; - this.isZkManaged = false; + this.isZkManaged = isZkManaged; this.zkc = zkc; this.sessionWatcher = new ZKSessionWatcher(zkc, this::receivedSessionEvent); zkc.addWatch("/", this::handleWatchEvent, AddWatchMode.PERSISTENT_RECURSIVE); @@ -478,7 +484,7 @@ public void close() throws Exception { private Stat getStat(String path, org.apache.zookeeper.data.Stat zkStat) { return new Stat(path, zkStat.getVersion(), zkStat.getCtime(), zkStat.getMtime(), - zkStat.getEphemeralOwner() != -1, + zkStat.getEphemeralOwner() != NOT_EPHEMERAL, zkStat.getEphemeralOwner() == zkc.getSessionId()); } diff --git a/pulsar-metadata/src/test/java/org/apache/bookkeeper/replication/BookKeeperClusterTestCase.java b/pulsar-metadata/src/test/java/org/apache/bookkeeper/replication/BookKeeperClusterTestCase.java index ccbdb8cef64c5..9724d2d6ff65d 100644 --- a/pulsar-metadata/src/test/java/org/apache/bookkeeper/replication/BookKeeperClusterTestCase.java +++ b/pulsar-metadata/src/test/java/org/apache/bookkeeper/replication/BookKeeperClusterTestCase.java @@ -212,6 +212,7 @@ public void tearDown() throws Exception { try { // cleanup for metrics. metadataStore.close(); + metadataStore = null; stopZKCluster(); } catch (Exception e) { LOG.error("Got Exception while trying to stop ZKCluster", e); @@ -256,6 +257,9 @@ private static void callCloseables(List closeables) { protected void startZKCluster() throws Exception { zkUtil.startCluster(); zkc = zkUtil.getZooKeeperClient(); + if (metadataStore != null) { + metadataStore.close(); + } metadataStore = new FaultInjectionMetadataStore( MetadataStoreExtended.create(zkUtil.getZooKeeperConnectString(), MetadataStoreConfig.builder().metadataStoreName("metastore-" + getClass().getSimpleName()).build())); diff --git a/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/BaseMetadataStoreTest.java b/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/BaseMetadataStoreTest.java index d0265e3ca44ee..03a6b91b62d4b 100644 --- a/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/BaseMetadataStoreTest.java +++ b/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/BaseMetadataStoreTest.java @@ -25,11 +25,18 @@ import io.streamnative.oxia.testcontainers.OxiaContainer; import java.io.File; import java.net.URI; +import java.util.Arrays; +import java.util.Set; import java.util.UUID; import java.util.concurrent.CompletionException; import java.util.function.Predicate; import java.util.function.Supplier; import java.util.stream.Collectors; +import org.apache.commons.lang3.StringUtils; +import org.apache.pulsar.metadata.api.MetadataStore; +import org.apache.pulsar.metadata.api.MetadataStoreConfig; +import org.apache.pulsar.metadata.api.MetadataStoreFactory; +import org.apache.pulsar.metadata.impl.MetadataStoreFactoryImpl; import org.apache.pulsar.tests.TestRetrySupport; import org.assertj.core.util.Files; import org.testng.annotations.AfterClass; @@ -37,21 +44,47 @@ import org.testng.annotations.DataProvider; public abstract class BaseMetadataStoreTest extends TestRetrySupport { + // to debug specific implementations, set the TEST_METADATA_PROVIDERS environment variable + // or temporarily hard code this value in the test class before running tests in the IDE + // supported values are ZooKeeper,Memory,RocksDB,Etcd,Oxia,MockZooKeeper + private static final String TEST_METADATA_PROVIDERS = System.getenv("TEST_METADATA_PROVIDERS"); + private static String originalMetadatastoreProvidersPropertyValue; protected TestZKServer zks; protected EtcdCluster etcdCluster; - protected OxiaContainer oxiaServer; + private String mockZkUrl; + // reference to keep the MockZooKeeper instance alive in MockZookeeperMetadataStoreProvider + private MetadataStore mockZkStoreRef; + private String zksConnectionString; + private String memoryConnectionString; + private String rocksdbConnectionString; + private File rocksDbDirectory; + private boolean running; @BeforeClass(alwaysRun = true) @Override public void setup() throws Exception { + running = true; incrementSetupNumber(); zks = new TestZKServer(); + zksConnectionString = zks.getConnectionString(); + memoryConnectionString = "memory:" + UUID.randomUUID(); + rocksDbDirectory = Files.newTemporaryFolder().getAbsoluteFile(); + rocksdbConnectionString = "rocksdb:" + rocksDbDirectory; + originalMetadatastoreProvidersPropertyValue = + System.getProperty(MetadataStoreFactoryImpl.METADATASTORE_PROVIDERS_PROPERTY); + // register MockZooKeeperMetadataStoreProvider + System.setProperty(MetadataStoreFactoryImpl.METADATASTORE_PROVIDERS_PROPERTY, + MockZooKeeperMetadataStoreProvider.class.getName()); + mockZkUrl = "mock-zk:" + UUID.randomUUID(); + // create a reference in MockZooKeeperMetadataStoreProvider to keep the MockZooKeeper instance alive + mockZkStoreRef = MetadataStoreFactory.create(mockZkUrl, MetadataStoreConfig.builder().build()); } @AfterClass(alwaysRun = true) @Override public void cleanup() throws Exception { + running = false; markCurrentSetupNumberCleaned(); if (zks != null) { zks.close(); @@ -67,40 +100,72 @@ public void cleanup() throws Exception { oxiaServer.close(); oxiaServer = null; } - } - private static String createTempFolder() { - File temp = Files.newTemporaryFolder(); - temp.deleteOnExit(); - return temp.getAbsolutePath(); + if (mockZkStoreRef != null) { + mockZkStoreRef.close(); + mockZkStoreRef = null; + mockZkUrl = null; + } + + if (rocksDbDirectory != null) { + Files.delete(rocksDbDirectory); + rocksDbDirectory = null; + } + + if (originalMetadatastoreProvidersPropertyValue != null) { + System.setProperty(MetadataStoreFactoryImpl.METADATASTORE_PROVIDERS_PROPERTY, + originalMetadatastoreProvidersPropertyValue); + } else { + System.clearProperty(MetadataStoreFactoryImpl.METADATASTORE_PROVIDERS_PROPERTY); + } } @DataProvider(name = "impl") public Object[][] implementations() { + // If the environment variable TEST_METADATA_PROVIDERS is set, only run the specified implementations + if (StringUtils.isNotBlank(TEST_METADATA_PROVIDERS)) { + return filterImplementations(TEST_METADATA_PROVIDERS.split(",")); + } + return allImplementations(); + } + + private Object[][] allImplementations() { // A Supplier must be used for the Zookeeper connection string parameter. The retried test run will // use the same arguments as the failed attempt. // The Zookeeper test server gets restarted by TestRetrySupport before the retry. // The new connection string won't be available to the test method unless a // Supplier lambda is used for providing the value. return new Object[][]{ - {"ZooKeeper", stringSupplier(() -> zks.getConnectionString())}, - {"Memory", stringSupplier(() -> "memory:" + UUID.randomUUID())}, - {"RocksDB", stringSupplier(() -> "rocksdb:" + createTempFolder())}, + {"ZooKeeper", stringSupplier(() -> zksConnectionString)}, + {"Memory", stringSupplier(() -> memoryConnectionString)}, + {"RocksDB", stringSupplier(() -> rocksdbConnectionString)}, {"Etcd", stringSupplier(() -> "etcd:" + getEtcdClusterConnectString())}, {"Oxia", stringSupplier(() -> "oxia://" + getOxiaServerConnectString())}, + {"MockZooKeeper", stringSupplier(() -> mockZkUrl)}, }; } @DataProvider(name = "distributedImpl") public Object[][] distributedImplementations() { - return new Object[][]{ - {"ZooKeeper", stringSupplier(() -> zks.getConnectionString())}, - {"Etcd", stringSupplier(() -> "etcd:" + getEtcdClusterConnectString())}, - {"Oxia", stringSupplier(() -> "oxia://" + getOxiaServerConnectString())}, - }; + return filterImplementations("ZooKeeper", "Etcd", "Oxia"); + } + + @DataProvider(name = "zkImpls") + public Object[][] zkImplementations() { + return filterImplementations("ZooKeeper", "MockZooKeeper"); + } + + protected Object[][] filterImplementations(String... providers) { + Set providersSet = Set.of(providers); + return Arrays.stream(allImplementations()) + .filter(impl -> providersSet.contains(impl[0])) + .toArray(Object[][]::new); } protected synchronized String getOxiaServerConnectString() { + if (!running) { + return null; + } if (oxiaServer == null) { oxiaServer = new OxiaContainer(OxiaContainer.DEFAULT_IMAGE_NAME); oxiaServer.start(); @@ -109,6 +174,9 @@ protected synchronized String getOxiaServerConnectString() { } private synchronized String getEtcdClusterConnectString() { + if (!running) { + return null; + } if (etcdCluster == null) { etcdCluster = EtcdClusterExtension.builder().withClusterName("test").withNodes(1).withSsl(false).build() .cluster(); @@ -118,7 +186,26 @@ private synchronized String getEtcdClusterConnectString() { } public static Supplier stringSupplier(Supplier supplier) { - return supplier; + return new StringSupplier(supplier); + } + + // Implements toString() so that the test name is more descriptive + private static class StringSupplier implements Supplier { + private final Supplier supplier; + + public StringSupplier(Supplier supplier) { + this.supplier = supplier; + } + + @Override + public String get() { + return supplier.get(); + } + + @Override + public String toString() { + return get(); + } } protected String newKey() { @@ -164,4 +251,15 @@ public static boolean retryStrategically(Predicate predicate, int retryCou } return false; } + + /** + * Delete all the empty container nodes + * @param provider the metadata store provider + * @throws Exception + */ + protected void maybeTriggerDeletingEmptyContainers(String provider) throws Exception { + if ("ZooKeeper".equals(provider) && zks != null) { + zks.checkContainers(); + } + } } diff --git a/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/CounterTest.java b/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/CounterTest.java index c5b4012f0c8f9..bd068539cc549 100644 --- a/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/CounterTest.java +++ b/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/CounterTest.java @@ -70,6 +70,7 @@ public void testCounterDoesNotAutoReset(String provider, Supplier urlSup return; } String metadataUrl = urlSupplier.get(); + @Cleanup MetadataStoreExtended store1 = MetadataStoreExtended.create(metadataUrl, MetadataStoreConfig.builder().build()); CoordinationService cs1 = new CoordinationServiceImpl(store1); @@ -85,7 +86,7 @@ public void testCounterDoesNotAutoReset(String provider, Supplier urlSup store1.close(); // Delete all the empty container nodes - zks.checkContainers(); + maybeTriggerDeletingEmptyContainers(provider); @Cleanup MetadataStoreExtended store2 = MetadataStoreExtended.create(metadataUrl, MetadataStoreConfig.builder().build()); diff --git a/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataCacheTest.java b/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataCacheTest.java index ddd975e422ab8..d3f2995a84505 100644 --- a/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataCacheTest.java +++ b/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataCacheTest.java @@ -71,7 +71,6 @@ import org.apache.pulsar.metadata.cache.impl.MetadataCacheImpl; import org.awaitility.Awaitility; import org.mockito.stubbing.Answer; -import org.testng.annotations.DataProvider; import org.testng.annotations.Test; @Slf4j @@ -113,14 +112,7 @@ public void emptyCacheTest(String provider, Supplier urlSupplier) throws } } - @DataProvider(name = "zk") - public Object[][] zkimplementations() { - return new Object[][] { - { "ZooKeeper", stringSupplier(() -> zks.getConnectionString()) }, - }; - } - - @Test(dataProvider = "zk") + @Test(dataProvider = "zkImpls") public void crossStoreAddDelete(String provider, Supplier urlSupplier) throws Exception { @Cleanup MetadataStore store1 = MetadataStoreFactory.create(urlSupplier.get(), MetadataStoreConfig.builder().build()); @@ -185,7 +177,7 @@ private void multiStoreAddDelete(List> caches, int addOn, }); } - @Test(dataProvider = "zk") + @Test(dataProvider = "zkImpls") public void crossStoreUpdates(String provider, Supplier urlSupplier) throws Exception { String testName = "cross store updates"; @Cleanup @@ -495,11 +487,10 @@ public void readModifyUpdate(String provider, Supplier urlSupplier) thro * * @throws Exception */ - @Test - public void readModifyUpdateBadVersionRetry() throws Exception { - String url = zks.getConnectionString(); + @Test(dataProvider = "zkImpls") + public void readModifyUpdateBadVersionRetry(String provider, Supplier urlSupplier) throws Exception { @Cleanup - MetadataStore store = MetadataStoreFactory.create(url, MetadataStoreConfig.builder().build()); + MetadataStore store = MetadataStoreFactory.create(urlSupplier.get(), MetadataStoreConfig.builder().build()); MetadataCache cache = store.getMetadataCache(MyClass.class); @@ -513,7 +504,8 @@ public void readModifyUpdateBadVersionRetry() throws Exception { final var sourceStores = new ArrayList(); for (int i = 0; i < 20; i++) { - final var sourceStore = MetadataStoreFactory.create(url, MetadataStoreConfig.builder().build()); + final var sourceStore = + MetadataStoreFactory.create(urlSupplier.get(), MetadataStoreConfig.builder().build()); sourceStores.add(sourceStore); final var objCache = sourceStore.getMetadataCache(MyClass.class); futures.add(objCache.readModifyUpdate(key1, v -> new MyClass(v.a, v.b + 1))); @@ -524,11 +516,10 @@ public void readModifyUpdateBadVersionRetry() throws Exception { } } - @Test - public void readModifyUpdateOrCreateRetryTimeout() throws Exception { - String url = zks.getConnectionString(); + @Test(dataProvider = "zkImpls") + public void readModifyUpdateOrCreateRetryTimeout(String provider, Supplier urlSupplier) throws Exception { @Cleanup - MetadataStore store = MetadataStoreFactory.create(url, MetadataStoreConfig.builder().build()); + MetadataStore store = MetadataStoreFactory.create(urlSupplier.get(), MetadataStoreConfig.builder().build()); MetadataCache cache = store.getMetadataCache(MyClass.class, MetadataCacheConfig.builder() .retryBackoff(new BackoffBuilder() diff --git a/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataStoreExtendedTest.java b/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataStoreExtendedTest.java index a4c937611fd3f..30fbd9b836e92 100644 --- a/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataStoreExtendedTest.java +++ b/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataStoreExtendedTest.java @@ -19,6 +19,7 @@ package org.apache.pulsar.metadata; import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertFalse; import static org.testng.Assert.assertNotEquals; import static org.testng.Assert.assertNotNull; import static org.testng.Assert.assertTrue; @@ -70,18 +71,19 @@ public void sequentialKeys(String provider, Supplier urlSupplier) throws @Test(dataProvider = "impl") public void testPersistentOrEphemeralPut(String provider, Supplier urlSupplier) throws Exception { final String key1 = newKey(); + @Cleanup MetadataStoreExtended store = MetadataStoreExtended.create(urlSupplier.get(), MetadataStoreConfig.builder().build()); store.put(key1, "value-1".getBytes(), Optional.empty(), EnumSet.noneOf(CreateOption.class)).join(); var value = store.get(key1).join().get(); assertEquals(value.getValue(), "value-1".getBytes()); - // assertFalse(value.getStat().isEphemeral()); // Todo : fix zkStat.getEphemeralOwner() != 0 from test zk + assertFalse(value.getStat().isEphemeral()); assertTrue(value.getStat().isFirstVersion()); var version = value.getStat().getVersion(); store.put(key1, "value-2".getBytes(), Optional.empty(), EnumSet.noneOf(CreateOption.class)).join(); value = store.get(key1).join().get(); assertEquals(value.getValue(), "value-2".getBytes()); - //assertFalse(value.getStat().isEphemeral()); // Todo : fix zkStat.getEphemeralOwner() != 0 from test zk + assertFalse(value.getStat().isEphemeral()); assertEquals(value.getStat().getVersion(), version + 1); final String key2 = newKey(); diff --git a/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataStoreTest.java b/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataStoreTest.java index 2c589dfd48222..a7b1dcf6bf02b 100644 --- a/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataStoreTest.java +++ b/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataStoreTest.java @@ -18,6 +18,7 @@ */ package org.apache.pulsar.metadata; +import static org.assertj.core.api.Assertions.assertThat; import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertFalse; import static org.testng.Assert.assertNotNull; @@ -68,6 +69,7 @@ import org.assertj.core.util.Lists; import org.awaitility.Awaitility; import org.awaitility.reflect.WhiteboxImpl; +import org.testng.SkipException; import org.testng.annotations.DataProvider; import org.testng.annotations.Test; @@ -415,8 +417,8 @@ public void testDeleteRecursive(String provider, Supplier urlSupplier) t @Test(dataProvider = "impl") public void testDeleteUnusedDirectories(String provider, Supplier urlSupplier) throws Exception { - if (provider.equals("Oxia")) { - return; + if (provider.equals("Oxia") || provider.equals("MockZooKeeper")) { + throw new SkipException("Oxia and MockZooKeeper do not support deleteUnusedDirectories"); } @Cleanup @@ -432,18 +434,18 @@ public void testDeleteUnusedDirectories(String provider, Supplier urlSup store.delete(prefix + "/a1/b1/c1", Optional.empty()).join(); store.delete(prefix + "/a1/b1/c2", Optional.empty()).join(); - zks.checkContainers(); + maybeTriggerDeletingEmptyContainers(provider); assertFalse(store.exists(prefix + "/a1/b1").join()); store.delete(prefix + "/a1/b2/c1", Optional.empty()).join(); - zks.checkContainers(); + maybeTriggerDeletingEmptyContainers(provider); assertFalse(store.exists(prefix + "/a1/b2").join()); - zks.checkContainers(); + maybeTriggerDeletingEmptyContainers(provider); assertFalse(store.exists(prefix + "/a1").join()); - zks.checkContainers(); + maybeTriggerDeletingEmptyContainers(provider); assertFalse(store.exists(prefix).join()); } @@ -549,6 +551,7 @@ public void testOxiaLoadConfigFromFile() throws Exception { builder.configFilePath("src/test/resources/oxia_client.conf"); MetadataStoreConfig config = builder.build(); + @Cleanup OxiaMetadataStore store = (OxiaMetadataStore) MetadataStoreFactory.create(oxia, config); var client = (AsyncOxiaClient) WhiteboxImpl.getInternalState(store, "client"); var sessionManager = (SessionManager) WhiteboxImpl.getInternalState(client, "sessionManager"); @@ -666,21 +669,25 @@ public void testGetChildren(String provider, Supplier urlSupplier) throw store.put("/b/c/b/1", "value1".getBytes(StandardCharsets.UTF_8), Optional.empty()).join(); List subPaths = store.getChildren("/").get(); - Set expectedSet = "ZooKeeper".equals(provider) ? Set.of("a", "b", "zookeeper") : Set.of("a", "b"); + Set ignoredRootPaths = Set.of("zookeeper"); + Set expectedSet = Set.of("a", "b"); for (String subPath : subPaths) { - assertTrue(expectedSet.contains(subPath)); + if (ignoredRootPaths.contains(subPath)) { + continue; + } + assertThat(expectedSet).contains(subPath); } List subPaths2 = store.getChildren("/a").get(); Set expectedSet2 = Set.of("a-1", "a-2"); for (String subPath : subPaths2) { - assertTrue(expectedSet2.contains(subPath)); + assertThat(expectedSet2).contains(subPath); } List subPaths3 = store.getChildren("/b").get(); Set expectedSet3 = Set.of("c"); for (String subPath : subPaths3) { - assertTrue(expectedSet3.contains(subPath)); + assertThat(expectedSet3).contains(subPath); } } diff --git a/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MockZooKeeperMetadataStoreProvider.java b/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MockZooKeeperMetadataStoreProvider.java new file mode 100644 index 0000000000000..994a97c2b1053 --- /dev/null +++ b/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MockZooKeeperMetadataStoreProvider.java @@ -0,0 +1,49 @@ +/* + * 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; + +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; +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.impl.ZKMetadataStore; +import org.apache.zookeeper.MockZooKeeper; +import org.apache.zookeeper.MockZooKeeperSession; + +public class MockZooKeeperMetadataStoreProvider implements MetadataStoreProvider { + private static final String MOCK_ZK_SCHEME = "mock-zk"; + private static final ConcurrentMap mockZooKeepers = new ConcurrentHashMap<>(); + + @Override + public String urlScheme() { + return MOCK_ZK_SCHEME; + } + + @Override + public MetadataStore create(String metadataURL, MetadataStoreConfig metadataStoreConfig, + boolean enableSessionWatcher) throws MetadataStoreException { + MockZooKeeper mockZooKeeper = mockZooKeepers.computeIfAbsent(metadataURL, + k -> MockZooKeeper.newInstance().registerCloseable(() -> mockZooKeepers.remove(k))); + MockZooKeeperSession mockZooKeeperSession = MockZooKeeperSession.newInstance(mockZooKeeper, true); + ZKMetadataStore zkMetadataStore = new ZKMetadataStore(mockZooKeeperSession, metadataStoreConfig, true); + return zkMetadataStore; + } +} diff --git a/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/bookkeeper/LedgerManagerIteratorTest.java b/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/bookkeeper/LedgerManagerIteratorTest.java index f8a51602686ed..f4bac7bb622aa 100644 --- a/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/bookkeeper/LedgerManagerIteratorTest.java +++ b/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/bookkeeper/LedgerManagerIteratorTest.java @@ -373,7 +373,7 @@ public void testWithSeveralIncompletePaths(String provider, Supplier url assertEquals(ledgersReadAsync, ids, "Comparing LedgersIds read asynchronously"); } - @Test(timeOut = 30000, dataProvider = "impl") + @Test(timeOut = 60000, dataProvider = "impl") public void checkConcurrentModifications(String provider, Supplier urlSupplier) throws Throwable { @Cleanup MetadataStoreExtended store = @@ -407,14 +407,16 @@ public void checkConcurrentModifications(String provider, Supplier urlSu ExecutorService executor = Executors.newCachedThreadPool(); final ConcurrentSkipListSet createdLedgers = new ConcurrentSkipListSet<>(); for (int i = 0; i < numWriters; ++i) { + int writerIndex = i; Future f = executor.submit(() -> { @Cleanup LedgerManager writerLM = new PulsarLedgerManager(store, ledgersRoot); Random writerRNG = new Random(rng.nextLong()); - + log.info("Writer {} waiting", writerIndex); latch.await(); - + log.info("Writer {} started", writerIndex); while (MathUtils.elapsedNanos(start) < runtime) { + log.info("Writer {} writing", writerIndex); long candidate = 0; do { candidate = Math.abs(writerRNG.nextLong()); @@ -426,18 +428,22 @@ public void checkConcurrentModifications(String provider, Supplier urlSu createLedger(writerLM, candidate); removeLedger(writerLM, candidate); } + log.info("Writer {} finished", writerIndex); return null; }); futures.add(f); } for (int i = 0; i < numCheckers; ++i) { + int checkerIndex = i; Future f = executor.submit(() -> { @Cleanup LedgerManager checkerLM = new PulsarLedgerManager(store, ledgersRoot); + log.info("Checker {} waiting", checkerIndex); latch.await(); - + log.info("Checker {} started", checkerIndex); while (MathUtils.elapsedNanos(start) < runtime) { + log.info("Checker {} checking", checkerIndex); LedgerRangeIterator lri = checkerLM.getLedgerRanges(0); Set returnedIds = ledgerRangeToSet(lri); for (long id : mustExist) { @@ -449,15 +455,19 @@ public void checkConcurrentModifications(String provider, Supplier urlSu assertTrue(ledgersReadAsync.contains(id)); } } + log.info("Checker {} finished", checkerIndex); return null; }); futures.add(f); } latch.countDown(); + log.info("Waiting for futures"); for (Future f : futures) { + log.info("Waiting for future"); f.get(); } + log.info("Completed"); executor.shutdownNow(); } diff --git a/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/bookkeeper/LedgerUnderreplicationManagerTest.java b/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/bookkeeper/LedgerUnderreplicationManagerTest.java index 0e9c781fb9143..ac73491a81c65 100644 --- a/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/bookkeeper/LedgerUnderreplicationManagerTest.java +++ b/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/bookkeeper/LedgerUnderreplicationManagerTest.java @@ -300,10 +300,10 @@ public void testMarkingAsReplicated(String provider, Supplier urlSupplie assertEquals(l, lB.get(), "Should be the ledger I marked"); } - - @Test(timeOut = 10000) - public void testZkMetasStoreMarkReplicatedDeleteEmptyParentNodes() throws Exception { - methodSetup(stringSupplier(() -> zks.getConnectionString())); + @Test(dataProvider = "zkImpls", timeOut = 10000) + public void testZkMetasStoreMarkReplicatedDeleteEmptyParentNodes(String provider, Supplier urlSupplier) + throws Exception { + methodSetup(urlSupplier); String missingReplica = "localhost:3181"; diff --git a/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerIdGeneratorTest.java b/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerIdGeneratorTest.java index 73d5f451c1ff1..da3fd7f7bd443 100644 --- a/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerIdGeneratorTest.java +++ b/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/bookkeeper/PulsarLedgerIdGeneratorTest.java @@ -242,7 +242,7 @@ public void testEnsureCounterIsNotResetWithContainerNodes(String provider, Suppl l1.await(); log.info("res1 : {}", res1); - zks.checkContainers(); + maybeTriggerDeletingEmptyContainers(provider); CountDownLatch l2 = new CountDownLatch(1); AtomicLong res2 = new AtomicLong(); diff --git a/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/impl/MetadataStoreFactoryImplTest.java b/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/impl/MetadataStoreFactoryImplTest.java index 6ede02b67136e..a0f726ed3dc9d 100644 --- a/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/impl/MetadataStoreFactoryImplTest.java +++ b/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/impl/MetadataStoreFactoryImplTest.java @@ -21,6 +21,10 @@ import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertTrue; import io.opentelemetry.api.OpenTelemetry; +import java.util.EnumSet; +import java.util.List; +import java.util.Optional; +import java.util.concurrent.CompletableFuture; import lombok.Cleanup; import org.apache.pulsar.metadata.api.GetResult; import org.apache.pulsar.metadata.api.MetadataStore; @@ -32,26 +36,25 @@ 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; + private static String originalMetadatastoreProvidersPropertyValue; @BeforeClass public void setMetadataStoreProperty() { - originalProperty = System.getProperties().get(MetadataStoreFactoryImpl.METADATASTORE_PROVIDERS_PROPERTY); + originalMetadatastoreProvidersPropertyValue = + System.getProperty(MetadataStoreFactoryImpl.METADATASTORE_PROVIDERS_PROPERTY); System.setProperty(MetadataStoreFactoryImpl.METADATASTORE_PROVIDERS_PROPERTY, MyMetadataStoreProvider.class.getName()); } @AfterClass public void resetMetadataStoreProperty() { - if (originalProperty != null) { - System.getProperties().put(MetadataStoreFactoryImpl.METADATASTORE_PROVIDERS_PROPERTY, originalProperty); + if (originalMetadatastoreProvidersPropertyValue != null) { + System.setProperty(MetadataStoreFactoryImpl.METADATASTORE_PROVIDERS_PROPERTY, + originalMetadatastoreProvidersPropertyValue); + } else { + System.clearProperty(MetadataStoreFactoryImpl.METADATASTORE_PROVIDERS_PROPERTY); } } diff --git a/testmocks/src/main/java/org/apache/zookeeper/MockZooKeeper.java b/testmocks/src/main/java/org/apache/zookeeper/MockZooKeeper.java index f32036e53f001..e124699ee1383 100644 --- a/testmocks/src/main/java/org/apache/zookeeper/MockZooKeeper.java +++ b/testmocks/src/main/java/org/apache/zookeeper/MockZooKeeper.java @@ -19,34 +19,32 @@ package org.apache.zookeeper; import com.google.common.collect.HashMultimap; -import com.google.common.collect.Iterables; -import com.google.common.collect.Lists; import com.google.common.collect.Maps; -import com.google.common.collect.Multimaps; import com.google.common.collect.SetMultimap; import com.google.common.collect.Sets; +import com.google.common.util.concurrent.MoreExecutors; import io.netty.util.concurrent.DefaultThreadFactory; import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; import java.util.List; +import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.TreeMap; -import java.util.TreeSet; +import java.util.concurrent.Callable; import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; +import java.util.concurrent.Future; import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; -import java.util.concurrent.locks.Lock; -import java.util.concurrent.locks.ReentrantLock; import java.util.function.BiPredicate; import lombok.AllArgsConstructor; -import lombok.Data; -import lombok.extern.slf4j.Slf4j; -import org.apache.commons.lang3.reflect.FieldUtils; import org.apache.zookeeper.AsyncCallback.Children2Callback; import org.apache.zookeeper.AsyncCallback.ChildrenCallback; import org.apache.zookeeper.AsyncCallback.DataCallback; @@ -57,6 +55,8 @@ import org.apache.zookeeper.Watcher.Event.KeeperState; import org.apache.zookeeper.data.ACL; import org.apache.zookeeper.data.Stat; +import org.apache.zookeeper.proto.DeleteRequest; +import org.apache.zookeeper.proto.SetDataRequest; import org.objenesis.Objenesis; import org.objenesis.ObjenesisStd; import org.objenesis.instantiator.ObjectInstantiator; @@ -64,33 +64,79 @@ import org.slf4j.LoggerFactory; public class MockZooKeeper extends ZooKeeper { - @Data + // ephemeralOwner value for persistent nodes + private static final long NOT_EPHEMERAL = 0L; + private static final String ROOT_PATH = "/"; + @AllArgsConstructor private static class MockZNode { byte[] content; int version; long ephemeralOwner; + long creationTimestamp; + long modificationTimestamp; + List children; static MockZNode of(byte[] content, int version, long ephemeralOwner) { - return new MockZNode(content, version, ephemeralOwner); + return new MockZNode(content, version, ephemeralOwner, System.currentTimeMillis(), + System.currentTimeMillis(), new ArrayList<>()); + } + + public void updateVersion() { + version++; + modificationTimestamp = System.currentTimeMillis(); + } + + public void updateData(byte[] data) { + content = data; + updateVersion(); + } + + public Stat getStat() { + return applyToStat(new Stat()); + } + + public Stat applyToStat(Stat stat) { + stat.setCtime(creationTimestamp); + stat.setMtime(modificationTimestamp); + stat.setVersion(version); + stat.setEphemeralOwner(ephemeralOwner); + return stat; + } + + public int getVersion() { + return version; + } + + public byte[] getContent() { + return content; + } + + public long getEphemeralOwner() { + return ephemeralOwner; + } + + public List getChildren() { + return children; } } private TreeMap tree; - private SetMultimap watchers; - private volatile boolean stopped; + private SetMultimap watchers; + private AtomicBoolean stopped; private AtomicReference alwaysFail; private CopyOnWriteArrayList failures; private ExecutorService executor; - private Watcher sessionWatcher; - private long sessionId = 0L; + private volatile Watcher sessionWatcher; + private long sessionId = 1L; private int readOpDelayMs; - private ReentrantLock mutex; - private AtomicLong sequentialIdGenerator; - private ThreadLocal epheralOwnerThreadLocal; + private ThreadLocal overriddenSessionIdThreadLocal; + private ThreadLocal inExecutorThreadLocal; + private int referenceCount; + private List closeables; //see details of Objenesis caching - http://objenesis.org/details.html //see supported jvms - https://github.com/easymock/objenesis/blob/master/SupportedJVMs.md @@ -110,41 +156,21 @@ private static class Failure { } } - @Data - @AllArgsConstructor - private static class PersistentWatcher { - final String path; - final Watcher watcher; - final AddWatchMode mode; + private record PersistentWatcher(String path, Watcher watcher, AddWatchMode mode, long sessionId) { } - private List persistentWatchers; - - public static MockZooKeeper newInstance() { - return newInstance(null); + private record NodeWatcher(Watcher watcher, long sessionId) { } - public static MockZooKeeper newInstance(ExecutorService executor) { - return newInstance(executor, -1); - } - - public static MockZooKeeper newInstanceForGlobalZK(ExecutorService executor) { - return newInstanceForGlobalZK(executor, -1); - } + private List persistentWatchers; - public static MockZooKeeper newInstanceForGlobalZK(ExecutorService executor, int readOpDelayMs) { - try { - return createMockZooKeeperInstance(executor, readOpDelayMs); - } catch (RuntimeException e) { - throw e; - } catch (Exception e) { - throw new IllegalStateException("Cannot create object", e); - } + public static MockZooKeeper newInstance() { + return newInstance(-1); } - public static MockZooKeeper newInstance(ExecutorService executor, int readOpDelayMs) { + public static MockZooKeeper newInstance(int readOpDelayMs) { try { - return createMockZooKeeperInstance(executor, readOpDelayMs); + return createMockZooKeeperInstance(readOpDelayMs); } catch (RuntimeException e) { throw e; } catch (Exception e) { @@ -152,29 +178,25 @@ public static MockZooKeeper newInstance(ExecutorService executor, int readOpDela } } - private static MockZooKeeper createMockZooKeeperInstance(ExecutorService executor, int readOpDelayMs) { + private static MockZooKeeper createMockZooKeeperInstance(int readOpDelayMs) { ObjectInstantiator mockZooKeeperInstantiator = objenesis.getInstantiatorOf(MockZooKeeper.class); MockZooKeeper zk = mockZooKeeperInstantiator.newInstance(); - zk.epheralOwnerThreadLocal = new ThreadLocal<>(); - zk.init(executor); + zk.overriddenSessionIdThreadLocal = new ThreadLocal<>(); + zk.inExecutorThreadLocal = ThreadLocal.withInitial(() -> false); + zk.init(); zk.readOpDelayMs = readOpDelayMs; - zk.mutex = new ReentrantLock(); - zk.lockInstance = ThreadLocal.withInitial(zk::createLock); zk.sequentialIdGenerator = new AtomicLong(); + zk.closeables = new ArrayList<>(); return zk; } - private void init(ExecutorService executor) { + private void init() { tree = Maps.newTreeMap(); - if (executor != null) { - this.executor = executor; - } else { - this.executor = Executors.newFixedThreadPool(1, new DefaultThreadFactory("mock-zookeeper")); - } - SetMultimap w = HashMultimap.create(); - watchers = Multimaps.synchronizedSetMultimap(w); - stopped = false; + tree.put(ROOT_PATH, MockZNode.of(new byte[0], 0, NOT_EPHEMERAL)); + this.executor = Executors.newSingleThreadExecutor(new DefaultThreadFactory("mock-zookeeper")); + watchers = HashMultimap.create(); + stopped = new AtomicBoolean(false); alwaysFail = new AtomicReference<>(KeeperException.Code.OK); failures = new CopyOnWriteArrayList<>(); persistentWatchers = new ArrayList<>(); @@ -197,101 +219,143 @@ public States getState() { return States.CONNECTED; } + @Override + public void register(Watcher watcher) { + sessionWatcher = watcher; + } - @Slf4j - private static class SingleAcquireAndReleaseLock { - private final AtomicBoolean acquired = new AtomicBoolean(false); - private final Lock lock; + @Override + public String create(String path, byte[] data, List acl, CreateMode createMode) + throws KeeperException, InterruptedException { + return runInExecutorReturningValue(() -> internalCreate(path, data, createMode)); + } - SingleAcquireAndReleaseLock(Lock lock) { - this.lock = lock; + private T runInExecutorReturningValue(Callable task) + throws InterruptedException, KeeperException { + if (isStopped()) { + throw new KeeperException.ConnectionLossException(); } - - public void lock() { - if (acquired.compareAndSet(false, true)) { - lock.lock(); - } else { - throw new IllegalStateException("Lock was already acquired!"); + if (inExecutorThreadLocal.get()) { + try { + return task.call(); + } catch (Exception e) { + if (e instanceof KeeperException ke) { + throw ke; + } + if (e instanceof InterruptedException ie) { + throw ie; + } + log.error("Unexpected exception", e); + throw new KeeperException.SystemErrorException(); } } - - public void unlockIfNeeded() { - if (acquired.compareAndSet(true, false)) { - lock.unlock(); + try { + long currentSessionId = getSessionId(); + return executor.submit(() -> { + inExecutorThreadLocal.set(true); + overrideSessionId(currentSessionId); + try { + return task.call(); + } finally { + removeSessionIdOverride(); + inExecutorThreadLocal.set(false); + } + }).get(); + } catch (ExecutionException e) { + Throwable cause = e.getCause(); + if (cause instanceof KeeperException ke) { + throw ke; + } + if (cause instanceof InterruptedException ie) { + throw ie; } + log.error("Unexpected exception", e); + throw new KeeperException.SystemErrorException(); } } - private ThreadLocal lockInstance; - - private SingleAcquireAndReleaseLock createLock() { - return new SingleAcquireAndReleaseLock(mutex); - } - - private void lock() { - lockInstance.get().lock(); - } - - private void unlockIfLocked() { - lockInstance.get().unlockIfNeeded(); + private void runInExecutorAsync(Runnable runnable) { + if (isStopped()) { + throw new RejectedExecutionException("MockZooKeeper is stopped"); + } + if (inExecutorThreadLocal.get()) { + try { + runnable.run(); + } catch (Throwable t) { + log.error("Unexpected exception", t); + } + return; + } + long currentSessionId = getSessionId(); + executor.submit(() -> { + try { + inExecutorThreadLocal.set(true); + overrideSessionId(currentSessionId); + try { + runnable.run(); + } finally { + removeSessionIdOverride(); + inExecutorThreadLocal.set(false); + } + } catch (Throwable t) { + log.error("Unexpected exception", t); + } + }); } - @Override - public void register(Watcher watcher) { - lock(); - sessionWatcher = watcher; - unlockIfLocked(); + private void runInExecutorSync(Runnable runnable) { + try { + runInExecutorReturningValue(() -> { + runnable.run(); + return null; + }); + } catch (Exception e) { + log.error("Unexpected error", e); + } } - @Override - public String create(String path, byte[] data, List acl, CreateMode createMode) - throws KeeperException, InterruptedException { + private String internalCreate(String path, byte[] data, CreateMode createMode) throws KeeperException { final Set toNotifyCreate = Sets.newHashSet(); final Set toNotifyParent = Sets.newHashSet(); - final String parent = path.substring(0, path.lastIndexOf("/")); - - lock(); - try { - + final String parent = getParentName(path); - maybeThrowProgrammedFailure(Op.CREATE, path); + maybeThrowProgrammedFailure(Op.CREATE, path); - if (stopped) { - throw new KeeperException.ConnectionLossException(); - } - - if (tree.containsKey(path)) { - throw new KeeperException.NodeExistsException(path); - } + if (isStopped()) { + throw new KeeperException.ConnectionLossException(); + } - if (!parent.isEmpty() && !tree.containsKey(parent)) { - throw new KeeperException.NoNodeException(); - } + if (tree.containsKey(path)) { + throw new KeeperException.NodeExistsException(path); + } - if (createMode.isSequential()) { - MockZNode parentNode = tree.get(parent); - int parentVersion = tree.get(parent).getVersion(); - path = path + parentVersion; + MockZNode parentNode = tree.get(parent); - // Update parent version - tree.put(parent, - MockZNode.of(parentNode.getContent(), parentVersion + 1, parentNode.getEphemeralOwner())); - } + if (parentNode == null) { + throw new KeeperException.NoNodeException(parent); + } - tree.put(path, MockZNode.of(data, 0, createMode.isEphemeral() ? getEphemeralOwner() : -1L)); + if (createMode.isSequential()) { + int parentVersion = parentNode.getVersion(); + path = path + parentVersion; + parentNode.updateVersion(); + } - toNotifyCreate.addAll(watchers.get(path)); + parentNode.getChildren().add(getNodeName(path)); + tree.put(path, createMockZNode(data, createMode)); - if (!parent.isEmpty()) { - toNotifyParent.addAll(watchers.get(parent)); - } - watchers.removeAll(path); - } finally { - unlockIfLocked(); + toNotifyCreate.addAll(getWatchers(path)); + if (!ROOT_PATH.equals(parent)) { + toNotifyParent.addAll(getWatchers(parent)); } + watchers.removeAll(path); final String finalPath = path; executor.execute(() -> { + if (isStopped()) { + return; + } + triggerPersistentWatches(finalPath, parent, EventType.NodeCreated); toNotifyCreate.forEach( @@ -309,43 +373,62 @@ public String create(String path, byte[] data, List acl, CreateMode createM return path; } - protected long getEphemeralOwner() { - Long epheralOwner = epheralOwnerThreadLocal.get(); - if (epheralOwner != null) { - return epheralOwner; + private static String getParentName(String path) { + String parentName = path.substring(0, path.lastIndexOf('/')); + return parentName.length() > 0 ? parentName : "/"; + } + + private static String getNodeName(String path) { + return path.substring(path.lastIndexOf('/') + 1); + } + + private Collection getWatchers(String path) { + Set nodeWatchers = watchers.get(path); + if (nodeWatchers != null) { + return nodeWatchers.stream().map(NodeWatcher::watcher).toList(); + } else { + return Collections.emptyList(); } - return getSessionId(); } - public void overrideEpheralOwner(long epheralOwner) { - epheralOwnerThreadLocal.set(epheralOwner); + @Override + public long getSessionId() { + Long overriddenSessionId = overriddenSessionIdThreadLocal.get(); + if (overriddenSessionId != null) { + return overriddenSessionId; + } + return sessionId; + } + + public void overrideSessionId(long sessionId) { + overriddenSessionIdThreadLocal.set(sessionId); } - public void removeEpheralOwnerOverride() { - epheralOwnerThreadLocal.remove(); + public void removeSessionIdOverride() { + overriddenSessionIdThreadLocal.remove(); } @Override public void create(final String path, final byte[] data, final List acl, CreateMode createMode, final StringCallback cb, final Object ctx) { - - - executor.execute(() -> { + if (isStopped()) { + cb.processResult(KeeperException.Code.CONNECTIONLOSS.intValue(), path, ctx, null); + return; + } + runInExecutorAsync(() -> { try { - lock(); - - if (stopped) { + if (isStopped()) { cb.processResult(KeeperException.Code.CONNECTIONLOSS.intValue(), path, ctx, null); return; } final Set toNotifyCreate = Sets.newHashSet(); - toNotifyCreate.addAll(watchers.get(path)); + toNotifyCreate.addAll(getWatchers(path)); final Set toNotifyParent = Sets.newHashSet(); - final String parent = path.substring(0, path.lastIndexOf("/")); - if (!parent.isEmpty()) { - toNotifyParent.addAll(watchers.get(parent)); + final String parent = getParentName(path); + if (!ROOT_PATH.equals(parent)) { + toNotifyParent.addAll(getWatchers(parent)); } final String name; @@ -357,355 +440,247 @@ public void create(final String path, final byte[] data, final List acl, Cr Optional failure = programmedFailure(Op.CREATE, path); if (failure.isPresent()) { - unlockIfLocked(); cb.processResult(failure.get().intValue(), path, ctx, null); - } else if (stopped) { - unlockIfLocked(); + } else if (isStopped()) { cb.processResult(KeeperException.Code.CONNECTIONLOSS.intValue(), path, ctx, null); } else if (tree.containsKey(path)) { - unlockIfLocked(); cb.processResult(KeeperException.Code.NODEEXISTS.intValue(), path, ctx, null); - } else if (!parent.isEmpty() && !tree.containsKey(parent)) { - unlockIfLocked(); - toNotifyParent.forEach(watcher -> watcher - .process(new WatchedEvent(EventType.NodeChildrenChanged, KeeperState.SyncConnected, - parent))); + } else if (!tree.containsKey(parent)) { + runNotifications(() -> { + toNotifyParent.forEach(watcher -> watcher + .process(new WatchedEvent(EventType.NodeChildrenChanged, KeeperState.SyncConnected, + parent))); + }); cb.processResult(KeeperException.Code.NONODE.intValue(), path, ctx, null); } else { - tree.put(name, MockZNode.of(data, 0, - createMode != null && createMode.isEphemeral() ? getEphemeralOwner() : -1L)); + tree.get(parent).getChildren().add(getNodeName(name)); + tree.put(name, createMockZNode(data, createMode)); watchers.removeAll(name); - unlockIfLocked(); cb.processResult(0, path, ctx, name); - - triggerPersistentWatches(path, parent, EventType.NodeCreated); - - toNotifyCreate.forEach( - watcher -> watcher.process( - new WatchedEvent(EventType.NodeCreated, - KeeperState.SyncConnected, - name))); - toNotifyParent.forEach( - watcher -> watcher.process( - new WatchedEvent(EventType.NodeChildrenChanged, - KeeperState.SyncConnected, - parent))); + runNotifications(() -> { + triggerPersistentWatches(path, parent, EventType.NodeCreated); + + toNotifyCreate.forEach( + watcher -> watcher.process( + new WatchedEvent(EventType.NodeCreated, + KeeperState.SyncConnected, + name))); + toNotifyParent.forEach( + watcher -> watcher.process( + new WatchedEvent(EventType.NodeChildrenChanged, + KeeperState.SyncConnected, + parent))); + }); } } catch (Throwable ex) { log.error("create path : {} error", path, ex); cb.processResult(KeeperException.Code.SYSTEMERROR.intValue(), path, ctx, null); - } finally { - unlockIfLocked(); } }); + } + + public void runNotifications(Runnable runnable) { + executor.execute(() -> { + if (isStopped()) { + return; + } + runnable.run(); + }); + } + private boolean isStopped() { + return stopped.get(); + } + + private MockZNode createMockZNode(byte[] data, CreateMode createMode) { + return MockZNode.of(data, 0, + createMode != null && createMode.isEphemeral() ? getSessionId() : NOT_EPHEMERAL); } @Override - public byte[] getData(String path, Watcher watcher, Stat stat) throws KeeperException { - lock(); - try { - maybeThrowProgrammedFailure(Op.GET, path); - MockZNode value = tree.get(path); - if (value == null) { - throw new KeeperException.NoNodeException(path); - } else { - if (watcher != null) { - watchers.put(path, watcher); - } - if (stat != null) { - applyToStat(value, stat); - } - return value.getContent(); + public byte[] getData(String path, Watcher watcher, Stat stat) throws KeeperException, InterruptedException { + return runInExecutorReturningValue(() -> internalGetData(path, watcher, stat)); + } + + private byte[] internalGetData(String path, Watcher watcher, Stat stat) throws KeeperException { + maybeThrowProgrammedFailure(Op.GET, path); + MockZNode value = tree.get(path); + if (value == null) { + throw new KeeperException.NoNodeException(path); + } else { + if (watcher != null) { + watchers.put(path, new NodeWatcher(watcher, getSessionId())); } - } finally { - unlockIfLocked(); + if (stat != null) { + value.applyToStat(stat); + } + return value.getContent(); } } @Override public void getData(final String path, boolean watch, final DataCallback cb, final Object ctx) { - executor.execute(() -> { - try { - checkReadOpDelay(); - Optional failure = programmedFailure(Op.GET, path); - if (failure.isPresent()) { - cb.processResult(failure.get().intValue(), path, ctx, null, null); - return; - } else if (stopped) { - cb.processResult(KeeperException.Code.CONNECTIONLOSS.intValue(), path, ctx, null, null); - return; - } - - MockZNode value; - lock(); - try { - value = tree.get(path); - } finally { - unlockIfLocked(); - } - - if (value == null) { - cb.processResult(KeeperException.Code.NONODE.intValue(), path, ctx, null, null); - } else { - cb.processResult(0, path, ctx, value.getContent(), createStatForZNode(value)); - } - } catch (Throwable ex) { - log.error("get data : {} error", path, ex); - cb.processResult(KeeperException.Code.SYSTEMERROR.intValue(), path, ctx, null, null); - } - }); + getData(path, null, cb, ctx); } @Override public void getData(final String path, final Watcher watcher, final DataCallback cb, final Object ctx) { - executor.execute(() -> { + if (isStopped()) { + cb.processResult(KeeperException.Code.CONNECTIONLOSS.intValue(), path, ctx, null, null); + return; + } + runInExecutorAsync(() -> { checkReadOpDelay(); try { - lock(); Optional failure = programmedFailure(Op.GET, path); if (failure.isPresent()) { - unlockIfLocked(); cb.processResult(failure.get().intValue(), path, ctx, null, null); return; - } else if (stopped) { - unlockIfLocked(); + } else if (isStopped()) { cb.processResult(KeeperException.Code.CONNECTIONLOSS.intValue(), path, ctx, null, null); return; } MockZNode value = tree.get(path); if (value == null) { - unlockIfLocked(); cb.processResult(KeeperException.Code.NONODE.intValue(), path, ctx, null, null); } else { if (watcher != null) { - watchers.put(path, watcher); + watchers.put(path, new NodeWatcher(watcher, getSessionId())); } - - Stat stat = createStatForZNode(value); - unlockIfLocked(); + Stat stat = value.getStat(); cb.processResult(0, path, ctx, value.getContent(), stat); } } catch (Throwable ex) { log.error("get data : {} error", path, ex); cb.processResult(KeeperException.Code.SYSTEMERROR.intValue(), path, ctx, null, null); - } finally { - unlockIfLocked(); } }); } @Override public void getChildren(final String path, final Watcher watcher, final ChildrenCallback cb, final Object ctx) { - executor.execute(() -> { - List children = Lists.newArrayList(); + if (isStopped()) { + cb.processResult(KeeperException.Code.CONNECTIONLOSS.intValue(), path, ctx, null); + return; + } + runInExecutorAsync(() -> { try { - lock(); Optional failure = programmedFailure(Op.GET_CHILDREN, path); if (failure.isPresent()) { - unlockIfLocked(); cb.processResult(failure.get().intValue(), path, ctx, null); return; - } else if (stopped) { - unlockIfLocked(); + } else if (isStopped()) { cb.processResult(KeeperException.Code.CONNECTIONLOSS.intValue(), path, ctx, null); return; } if (!tree.containsKey(path)) { - unlockIfLocked(); cb.processResult(KeeperException.Code.NONODE.intValue(), path, ctx, null); return; } - for (String item : tree.tailMap(path).keySet()) { - if (!item.startsWith(path)) { - break; - } else { - if (path.length() >= item.length()) { - continue; - } - - String child = item.substring(path.length() + 1); - if (item.charAt(path.length()) == '/' && !child.contains("/")) { - children.add(child); - } - } - } - + List children = findFirstLevelChildren(path); if (watcher != null) { - watchers.put(path, watcher); + watchers.put(path, new NodeWatcher(watcher, getSessionId())); } cb.processResult(0, path, ctx, children); } catch (Throwable ex) { log.error("get children : {} error", path, ex); cb.processResult(KeeperException.Code.SYSTEMERROR.intValue(), path, ctx, null); - } finally { - unlockIfLocked(); } - }); } @Override - public List getChildren(String path, Watcher watcher) throws KeeperException { - lock(); - try { - maybeThrowProgrammedFailure(Op.GET_CHILDREN, path); - - if (!tree.containsKey(path)) { - throw new KeeperException.NoNodeException(); - } - - String firstKey = path.equals("/") ? path : path + "/"; - String lastKey = path.equals("/") ? "0" : path + "0"; // '0' is lexicographically just after '/' + public List getChildren(String path, Watcher watcher) throws KeeperException, InterruptedException { + return runInExecutorReturningValue(() -> internalGetChildren(path, watcher)); + } - Set children = new TreeSet<>(); - tree.subMap(firstKey, false, lastKey, false).forEach((key, value) -> { - String relativePath = key.replace(firstKey, ""); + private List internalGetChildren(String path, Watcher watcher) throws KeeperException { + maybeThrowProgrammedFailure(Op.GET_CHILDREN, path); - // Only return first-level children - String child = relativePath.split("/", 2)[0]; - children.add(child); - }); - - if (watcher != null) { - watchers.put(path, watcher); - } + if (!tree.containsKey(path)) { + throw new KeeperException.NoNodeException(path); + } - return new ArrayList<>(children); - } finally { - unlockIfLocked(); + if (watcher != null) { + watchers.put(path, new NodeWatcher(watcher, getSessionId())); } + + return findFirstLevelChildren(path); } @Override public List getChildren(String path, boolean watch) throws KeeperException, InterruptedException { - lock(); - try { - maybeThrowProgrammedFailure(Op.GET_CHILDREN, path); - - if (stopped) { - throw new KeeperException.ConnectionLossException(); - } else if (!tree.containsKey(path)) { - throw new KeeperException.NoNodeException(); - } - - String firstKey = path.equals("/") ? path : path + "/"; - String lastKey = path.equals("/") ? "0" : path + "0"; // '0' is lexicographically just after '/' - - Set children = new TreeSet<>(); - tree.subMap(firstKey, false, lastKey, false).forEach((key, value) -> { - String relativePath = key.replace(firstKey, ""); - - // Only return first-level children - String child = relativePath.split("/", 2)[0]; - children.add(child); - }); - - return new ArrayList<>(children); - } finally { - unlockIfLocked(); - } + return getChildren(path, null); } @Override public void getChildren(final String path, boolean watcher, final Children2Callback cb, final Object ctx) { - executor.execute(() -> { - Set children = new TreeSet<>(); + if (isStopped()) { + cb.processResult(KeeperException.Code.CONNECTIONLOSS.intValue(), path, ctx, null, null); + return; + } + runInExecutorAsync(() -> { try { - lock(); + MockZNode mockZNode = tree.get(path); + Stat stat = mockZNode != null ? mockZNode.getStat() : null; Optional failure = programmedFailure(Op.GET_CHILDREN, path); if (failure.isPresent()) { - unlockIfLocked(); cb.processResult(failure.get().intValue(), path, ctx, null, null); return; - } else if (stopped) { - unlockIfLocked(); + } else if (isStopped()) { cb.processResult(KeeperException.Code.CONNECTIONLOSS.intValue(), path, ctx, null, null); return; - } else if (!tree.containsKey(path)) { - unlockIfLocked(); + } else if (mockZNode == null) { cb.processResult(KeeperException.Code.NONODE.intValue(), path, ctx, null, null); return; } - String firstKey = path.equals("/") ? path : path + "/"; - String lastKey = path.equals("/") ? "0" : path + "0"; // '0' is lexicographically just after '/' - - tree.subMap(firstKey, false, lastKey, false).forEach((key, value) -> { - String relativePath = key.replace(firstKey, ""); - - // Only return first-level children - String child = relativePath.split("/", 2)[0]; - children.add(child); - }); - cb.processResult(0, path, ctx, new ArrayList<>(children), new Stat()); + List children = findFirstLevelChildren(path); + cb.processResult(0, path, ctx, children, stat); } catch (Throwable ex) { log.error("get children : {} error", path, ex); cb.processResult(KeeperException.Code.SYSTEMERROR.intValue(), path, ctx, null, null); - } finally { - unlockIfLocked(); } }); + } + private List findFirstLevelChildren(String path) { + return new ArrayList<>(tree.get(path).getChildren()); + } + + private boolean hasChildren(String path) { + return !tree.get(path).getChildren().isEmpty(); } @Override public Stat exists(String path, boolean watch) throws KeeperException, InterruptedException { - lock(); - try { - maybeThrowProgrammedFailure(Op.EXISTS, path); + return runInExecutorReturningValue(() -> internalGetStat(path, null)); + } - if (stopped) { - throw new KeeperException.ConnectionLossException(); - } + private Stat internalGetStat(String path, Watcher watcher) throws KeeperException { + maybeThrowProgrammedFailure(Op.EXISTS, path); - if (tree.containsKey(path)) { - return createStatForZNode(tree.get(path)); - } else { - return null; - } - } finally { - unlockIfLocked(); + if (isStopped()) { + throw new KeeperException.ConnectionLossException(); } - } - private static Stat createStatForZNode(MockZNode zNode) { - return applyToStat(zNode, new Stat()); - } + if (watcher != null) { + watchers.put(path, new NodeWatcher(watcher, getSessionId())); + } - private static Stat applyToStat(MockZNode zNode, Stat stat) { - stat.setVersion(zNode.getVersion()); - if (zNode.getEphemeralOwner() != -1L) { - stat.setEphemeralOwner(zNode.getEphemeralOwner()); + if (tree.containsKey(path)) { + return tree.get(path).getStat(); + } else { + return null; } - return stat; } @Override public Stat exists(String path, Watcher watcher) throws KeeperException, InterruptedException { - lock(); - try { - maybeThrowProgrammedFailure(Op.EXISTS, path); - - if (stopped) { - throw new KeeperException.ConnectionLossException(); - } - - if (watcher != null) { - watchers.put(path, watcher); - } - - if (tree.containsKey(path)) { - return createStatForZNode(tree.get(path)); - } else { - return null; - } - } finally { - unlockIfLocked(); - } + return runInExecutorReturningValue(() -> internalGetStat(path, watcher)); } @Override @@ -715,160 +690,149 @@ public void exists(String path, boolean watch, StatCallback cb, Object ctx) { @Override public void exists(String path, Watcher watcher, StatCallback cb, Object ctx) { - executor.execute(() -> { + if (isStopped()) { + cb.processResult(KeeperException.Code.CONNECTIONLOSS.intValue(), path, ctx, null); + return; + } + runInExecutorAsync(() -> { try { - lock(); Optional failure = programmedFailure(Op.EXISTS, path); if (failure.isPresent()) { - unlockIfLocked(); cb.processResult(failure.get().intValue(), path, ctx, null); return; - } else if (stopped) { - unlockIfLocked(); + } else if (isStopped()) { cb.processResult(KeeperException.Code.CONNECTIONLOSS.intValue(), path, ctx, null); return; } if (watcher != null) { - watchers.put(path, watcher); + watchers.put(path, new NodeWatcher(watcher, getSessionId())); } - if (tree.containsKey(path)) { - unlockIfLocked(); - cb.processResult(0, path, ctx, new Stat()); + MockZNode mockZNode = tree.get(path); + if (mockZNode != null) { + Stat stat = mockZNode.getStat(); + cb.processResult(0, path, ctx, stat); } else { - unlockIfLocked(); cb.processResult(KeeperException.Code.NONODE.intValue(), path, ctx, null); } } catch (Throwable ex) { log.error("exist : {} error", path, ex); cb.processResult(KeeperException.Code.SYSTEMERROR.intValue(), path, ctx, null); - } finally { - unlockIfLocked(); } }); } @Override public void sync(String path, VoidCallback cb, Object ctx) { - executor.execute(() -> { + if (isStopped()) { + cb.processResult(KeeperException.Code.CONNECTIONLOSS.intValue(), path, ctx); + return; + } + runInExecutorAsync(() -> { Optional failure = programmedFailure(Op.SYNC, path); if (failure.isPresent()) { cb.processResult(failure.get().intValue(), path, ctx); return; - } else if (stopped) { + } else if (isStopped()) { cb.processResult(KeeperException.Code.CONNECTIONLOSS.intValue(), path, ctx); return; } - cb.processResult(0, path, ctx); }); - } @Override public Stat setData(final String path, byte[] data, int version) throws KeeperException, InterruptedException { - final Set toNotify = Sets.newHashSet(); - MockZNode newZNode; - - lock(); - try { - maybeThrowProgrammedFailure(Op.SET, path); - - if (stopped) { - throw new KeeperException.ConnectionLossException(); - } + return runInExecutorReturningValue(() -> internalSetData(path, data, version)); + } - if (!tree.containsKey(path)) { - throw new KeeperException.NoNodeException(); - } + private Stat internalSetData(String path, byte[] data, int version) throws KeeperException { + final Set toNotify = Sets.newHashSet(); + maybeThrowProgrammedFailure(Op.SET, path); - MockZNode mockZNode = tree.get(path); - int currentVersion = mockZNode.getVersion(); + if (isStopped()) { + throw new KeeperException.ConnectionLossException(); + } - // Check version - if (version != -1 && version != currentVersion) { - throw new KeeperException.BadVersionException(path); - } + if (!tree.containsKey(path)) { + throw new KeeperException.NoNodeException(path); + } - log.debug("[{}] Updating -- current version: {}", path, currentVersion); - newZNode = MockZNode.of(data, currentVersion + 1, mockZNode.getEphemeralOwner()); - tree.put(path, newZNode); + MockZNode mockZNode = tree.get(path); + int currentVersion = mockZNode.getVersion(); - toNotify.addAll(watchers.get(path)); - watchers.removeAll(path); - } finally { - unlockIfLocked(); + // Check version + if (version != -1 && version != currentVersion) { + throw new KeeperException.BadVersionException(path); } - executor.execute(() -> { + log.debug("[{}] Updating -- current version: {}", path, currentVersion); + mockZNode.updateData(data); + Stat stat = mockZNode.getStat(); + toNotify.addAll(getWatchers(path)); + watchers.removeAll(path); + + runNotifications(() -> { triggerPersistentWatches(path, null, EventType.NodeDataChanged); toNotify.forEach(watcher -> watcher .process(new WatchedEvent(EventType.NodeDataChanged, KeeperState.SyncConnected, path))); }); - return createStatForZNode(newZNode); + return stat; } @Override public void setData(final String path, final byte[] data, int version, final StatCallback cb, final Object ctx) { - if (stopped) { + if (isStopped()) { cb.processResult(KeeperException.Code.CONNECTIONLOSS.intValue(), path, ctx, null); return; } - - executor.execute(() -> { + runInExecutorAsync(() -> { try { final Set toNotify = Sets.newHashSet(); Stat stat; - lock(); - try { - Optional failure = programmedFailure(Op.SET, path); - if (failure.isPresent()) { - unlockIfLocked(); - cb.processResult(failure.get().intValue(), path, ctx, null); - return; - } else if (stopped) { - unlockIfLocked(); - cb.processResult(KeeperException.Code.CONNECTIONLOSS.intValue(), path, ctx, null); - return; - } - - if (!tree.containsKey(path)) { - unlockIfLocked(); - cb.processResult(KeeperException.Code.NONODE.intValue(), path, ctx, null); - return; - } + Optional failure = programmedFailure(Op.SET, path); + if (failure.isPresent()) { + cb.processResult(failure.get().intValue(), path, ctx, null); + return; + } else if (isStopped()) { + cb.processResult(KeeperException.Code.CONNECTIONLOSS.intValue(), path, ctx, null); + return; + } - MockZNode mockZNode = tree.get(path); - int currentVersion = mockZNode.getVersion(); + if (!tree.containsKey(path)) { + cb.processResult(KeeperException.Code.NONODE.intValue(), path, ctx, null); + return; + } - // Check version - if (version != -1 && version != currentVersion) { - log.debug("[{}] Current version: {} -- Expected: {}", path, currentVersion, version); - unlockIfLocked(); - cb.processResult(KeeperException.Code.BADVERSION.intValue(), path, ctx, null); - return; - } + MockZNode mockZNode = tree.get(path); + int currentVersion = mockZNode.getVersion(); - log.debug("[{}] Updating -- current version: {}", path, currentVersion); - MockZNode newZNode = MockZNode.of(data, currentVersion + 1, mockZNode.getEphemeralOwner()); - tree.put(path, newZNode); - stat = createStatForZNode(newZNode); - } finally { - unlockIfLocked(); + // Check version + if (version != -1 && version != currentVersion) { + log.debug("[{}] Current version: {} -- Expected: {}", path, currentVersion, version); + Stat currentStat = mockZNode.getStat(); + cb.processResult(KeeperException.Code.BADVERSION.intValue(), path, ctx, currentStat); + return; } + + log.debug("[{}] Updating -- current version: {}", path, currentVersion); + mockZNode.updateData(data); + stat = mockZNode.getStat(); cb.processResult(0, path, ctx, stat); - toNotify.addAll(watchers.get(path)); + toNotify.addAll(getWatchers(path)); watchers.removeAll(path); - for (Watcher watcher : toNotify) { - watcher.process(new WatchedEvent(EventType.NodeDataChanged, KeeperState.SyncConnected, path)); - } + runNotifications(() -> { + triggerPersistentWatches(path, null, EventType.NodeDataChanged); - triggerPersistentWatches(path, null, EventType.NodeDataChanged); + for (Watcher watcher : toNotify) { + watcher.process(new WatchedEvent(EventType.NodeDataChanged, KeeperState.SyncConnected, path)); + } + }); } catch (Throwable ex) { log.error("Update data : {} error", path, ex); cb.processResult(KeeperException.Code.SYSTEMERROR.intValue(), path, ctx, null); @@ -878,50 +842,49 @@ public void setData(final String path, final byte[] data, int version, final Sta @Override public void delete(final String path, int version) throws InterruptedException, KeeperException { + runInExecutorReturningValue(() -> { + internalDelete(path, version); + return null; + }); + } + + private void internalDelete(String path, int version) throws KeeperException { maybeThrowProgrammedFailure(Op.DELETE, path); final Set toNotifyDelete; final Set toNotifyParent; final String parent; - lock(); - try { - if (stopped) { - throw new KeeperException.ConnectionLossException(); - } else if (!tree.containsKey(path)) { - throw new KeeperException.NoNodeException(path); - } else if (hasChildren(path)) { - throw new KeeperException.NotEmptyException(path); - } + if (isStopped()) { + throw new KeeperException.ConnectionLossException(); + } else if (!tree.containsKey(path)) { + throw new KeeperException.NoNodeException(path); + } else if (hasChildren(path)) { + throw new KeeperException.NotEmptyException(path); + } - if (version != -1) { - int currentVersion = tree.get(path).getVersion(); - if (version != currentVersion) { - throw new KeeperException.BadVersionException(path); - } + if (version != -1) { + int currentVersion = tree.get(path).getVersion(); + if (version != currentVersion) { + throw new KeeperException.BadVersionException(path); } + } - tree.remove(path); - - toNotifyDelete = Sets.newHashSet(); - toNotifyDelete.addAll(watchers.get(path)); + parent = getParentName(path); + tree.remove(path); + tree.get(parent).getChildren().remove(getNodeName(path)); - toNotifyParent = Sets.newHashSet(); - parent = path.substring(0, path.lastIndexOf("/")); - if (!parent.isEmpty()) { - toNotifyParent.addAll(watchers.get(parent)); - } + toNotifyDelete = Sets.newHashSet(); + toNotifyDelete.addAll(getWatchers(path)); - watchers.removeAll(path); - } finally { - unlockIfLocked(); + toNotifyParent = Sets.newHashSet(); + if (!ROOT_PATH.equals(parent)) { + toNotifyParent.addAll(getWatchers(parent)); } - executor.execute(() -> { - if (stopped) { - return; - } + watchers.removeAll(path); + runNotifications(() -> { for (Watcher watcher1 : toNotifyDelete) { watcher1.process(new WatchedEvent(EventType.NodeDeleted, KeeperState.SyncConnected, path)); } @@ -935,179 +898,209 @@ public void delete(final String path, int version) throws InterruptedException, @Override public void delete(final String path, int version, final VoidCallback cb, final Object ctx) { - Runnable r = () -> { + if (isStopped()) { + cb.processResult(KeeperException.Code.CONNECTIONLOSS.intValue(), path, ctx); + return; + } + runInExecutorAsync(() -> { try { - lock(); final Set toNotifyDelete = Sets.newHashSet(); - toNotifyDelete.addAll(watchers.get(path)); + toNotifyDelete.addAll(getWatchers(path)); final Set toNotifyParent = Sets.newHashSet(); - final String parent = path.substring(0, path.lastIndexOf("/")); - if (!parent.isEmpty()) { - toNotifyParent.addAll(watchers.get(parent)); + final String parent = getParentName(path); + if (!ROOT_PATH.equals(parent)) { + toNotifyParent.addAll(getWatchers(parent)); } watchers.removeAll(path); Optional failure = programmedFailure(Op.DELETE, path); if (failure.isPresent()) { - unlockIfLocked(); cb.processResult(failure.get().intValue(), path, ctx); - } else if (stopped) { - unlockIfLocked(); + } else if (isStopped()) { cb.processResult(KeeperException.Code.CONNECTIONLOSS.intValue(), path, ctx); } else if (!tree.containsKey(path)) { - unlockIfLocked(); cb.processResult(KeeperException.Code.NONODE.intValue(), path, ctx); } else if (hasChildren(path)) { - unlockIfLocked(); cb.processResult(KeeperException.Code.NOTEMPTY.intValue(), path, ctx); } else { if (version != -1) { int currentVersion = tree.get(path).getVersion(); if (version != currentVersion) { - unlockIfLocked(); cb.processResult(KeeperException.Code.BADVERSION.intValue(), path, ctx); return; } } tree.remove(path); - - unlockIfLocked(); + tree.get(parent).getChildren().remove(getNodeName(path)); cb.processResult(0, path, ctx); - toNotifyDelete.forEach(watcher -> watcher - .process(new WatchedEvent(EventType.NodeDeleted, KeeperState.SyncConnected, path))); - toNotifyParent.forEach(watcher -> watcher - .process(new WatchedEvent(EventType.NodeChildrenChanged, KeeperState.SyncConnected, - parent))); - triggerPersistentWatches(path, parent, EventType.NodeDeleted); + runNotifications(() -> { + triggerPersistentWatches(path, parent, EventType.NodeDeleted); + toNotifyDelete.forEach(watcher -> watcher + .process(new WatchedEvent(EventType.NodeDeleted, KeeperState.SyncConnected, path))); + toNotifyParent.forEach(watcher -> watcher + .process(new WatchedEvent(EventType.NodeChildrenChanged, KeeperState.SyncConnected, + parent))); + }); } } catch (Throwable ex) { log.error("delete path : {} error", path, ex); cb.processResult(KeeperException.Code.SYSTEMERROR.intValue(), path, ctx); - } finally { - unlockIfLocked(); } - }; - - try { - executor.execute(r); - } catch (RejectedExecutionException ree) { - cb.processResult(KeeperException.Code.SESSIONEXPIRED.intValue(), path, ctx); - } - + }); } @Override public void multi(Iterable ops, AsyncCallback.MultiCallback cb, Object ctx) { - try { - List res = multi(ops); - cb.processResult(KeeperException.Code.OK.intValue(), null, ctx, res); - } catch (Exception e) { - cb.processResult(KeeperException.Code.APIERROR.intValue(), null, ctx, null); + if (isStopped()) { + cb.processResult(KeeperException.Code.CONNECTIONLOSS.intValue(), null, ctx, null); + return; } + runInExecutorAsync(() -> { + try { + List res = multi(ops); + cb.processResult(KeeperException.Code.OK.intValue(), null, ctx, res); + } catch (Exception e) { + cb.processResult(KeeperException.Code.APIERROR.intValue(), null, ctx, null); + } + }); } @Override public List multi(Iterable ops) throws InterruptedException, KeeperException { + return runInExecutorReturningValue(() -> internalMulti(ops)); + } + + private List internalMulti(Iterable ops) { List res = new ArrayList<>(); - try { - for (org.apache.zookeeper.Op op : ops) { - switch (op.getType()) { - case ZooDefs.OpCode.create -> { + for (org.apache.zookeeper.Op op : ops) { + switch (op.getType()) { + case ZooDefs.OpCode.create -> { + handleOperation("create", op, () -> { org.apache.zookeeper.Op.Create opc = ((org.apache.zookeeper.Op.Create) op); CreateMode cm = CreateMode.fromFlag(opc.flags); - String path = this.create(op.getPath(), opc.data, null, cm); + String path = create(op.getPath(), opc.data, null, cm); res.add(new OpResult.CreateResult(path)); - } - case ZooDefs.OpCode.delete -> { - this.delete(op.getPath(), (int) FieldUtils.readField(op, "version", true)); + }, res); + } + case ZooDefs.OpCode.delete -> { + handleOperation("delete", op, () -> { + DeleteRequest deleteRequest = (DeleteRequest) op.toRequestRecord(); + delete(op.getPath(), deleteRequest.getVersion()); res.add(new OpResult.DeleteResult()); - } - case ZooDefs.OpCode.setData -> { - Stat stat = this.setData( - op.getPath(), - (byte[]) FieldUtils.readField(op, "data", true), - (int) FieldUtils.readField(op, "version", true)); + }, res); + } + case ZooDefs.OpCode.setData -> { + handleOperation("setData", op, () -> { + SetDataRequest setDataRequest = (SetDataRequest) op.toRequestRecord(); + Stat stat = setData(op.getPath(), setDataRequest.getData(), setDataRequest.getVersion()); res.add(new OpResult.SetDataResult(stat)); - } - case ZooDefs.OpCode.getChildren -> { - try { - List children = this.getChildren(op.getPath(), null); - res.add(new OpResult.GetChildrenResult(children)); - } catch (KeeperException e) { - res.add(new OpResult.ErrorResult(e.code().intValue())); - } - } - case ZooDefs.OpCode.getData -> { - Stat stat = new Stat(); - try { - byte[] payload = this.getData(op.getPath(), null, stat); - res.add(new OpResult.GetDataResult(payload, stat)); - } catch (KeeperException e) { - res.add(new OpResult.ErrorResult(e.code().intValue())); - } - } + }, res); + } + case ZooDefs.OpCode.getChildren -> { + handleOperation("getChildren", op, () -> { + List children = getChildren(op.getPath(), null); + res.add(new OpResult.GetChildrenResult(children)); + }, res); + } + case ZooDefs.OpCode.getData -> { + Stat stat = new Stat(); + handleOperation("getData", op, () -> { + byte[] payload = getData(op.getPath(), null, stat); + res.add(new OpResult.GetDataResult(payload, stat)); + }, res); + } + default -> { + log.error("Unsupported operation for path {} type {} kind {} request {}", op.getPath(), + op.getType(), op.getKind(), op.toRequestRecord()); + res.add(new OpResult.ErrorResult(KeeperException.Code.APIERROR.intValue())); } } - } catch (KeeperException e) { - res.add(new OpResult.ErrorResult(e.code().intValue())); - int total = Iterables.size(ops); - for (int i = res.size(); i < total; i++) { + } + return res; + } + + interface ZkOpHandler { + void handle() throws KeeperException, InterruptedException; + } + + private void handleOperation(String opName, org.apache.zookeeper.Op op, ZkOpHandler handler, List res) { + try { + handler.handle(); + } catch (Exception e) { + if (e instanceof KeeperException keeperException) { + res.add(new OpResult.ErrorResult(keeperException.code().intValue())); + } else { + log.error("Error handling {} operation for path {} type {} kind {} request {}", opName, op.getPath(), + op.getType(), op.getKind(), op.toRequestRecord(), e); res.add(new OpResult.ErrorResult(KeeperException.Code.RUNTIMEINCONSISTENCY.intValue())); } - } catch (IllegalAccessException e) { - throw new IllegalStateException(e); } - return res; } @Override - public synchronized void addWatch(String basePath, Watcher watcher, AddWatchMode mode) { - persistentWatchers.add(new PersistentWatcher(basePath, watcher, mode)); + public void addWatch(String basePath, Watcher watcher, AddWatchMode mode) { + runInExecutorSync(() -> { + persistentWatchers.add(new PersistentWatcher(basePath, watcher, mode, getSessionId())); + }); } @Override public void addWatch(String basePath, Watcher watcher, AddWatchMode mode, VoidCallback cb, Object ctx) { - if (stopped) { + if (isStopped()) { cb.processResult(KeeperException.Code.CONNECTIONLOSS.intValue(), basePath, ctx); return; } - - executor.execute(() -> { - synchronized (MockZooKeeper.this) { - persistentWatchers.add(new PersistentWatcher(basePath, watcher, mode)); - } - + runInExecutorAsync(() -> { + addWatch(basePath, watcher, mode); cb.processResult(KeeperException.Code.OK.intValue(), basePath, ctx); }); } + public synchronized void increaseRefCount() { + referenceCount++; + } + + public synchronized MockZooKeeper registerCloseable(AutoCloseable closeable) { + closeables.add(closeable); + return this; + } + @Override - public void close() throws InterruptedException { - shutdown(); + public synchronized void close() throws InterruptedException { + if (--referenceCount <= 0) { + shutdown(); + closeables.forEach(c -> { + try { + c.close(); + } catch (Exception e) { + log.error("Error closing closeable", e); + } + }); + closeables.clear(); + } } public void shutdown() throws InterruptedException { - lock(); - try { - stopped = true; - tree.clear(); - watchers.clear(); + if (stopped.compareAndSet(false, true)) { + Future shutdownTask = executor.submit(() -> { + tree.clear(); + watchers.clear(); + persistentWatchers.clear(); + }); try { - executor.shutdownNow(); - executor.awaitTermination(5, TimeUnit.SECONDS); - } catch (InterruptedException ex) { - log.error("MockZooKeeper shutdown had error", ex); + shutdownTask.get(); + } catch (ExecutionException e) { + log.error("Error shutting down", e); } - } finally { - unlockIfLocked(); + MoreExecutors.shutdownAndAwaitTermination(executor, 10, TimeUnit.SECONDS); } } Optional programmedFailure(Op op, String path) { - KeeperException.Code error = this.alwaysFail.get(); + KeeperException.Code error = alwaysFail.get(); if (error != KeeperException.Code.OK) { return Optional.of(error); } @@ -1144,26 +1137,17 @@ public void delay(long millis, BiPredicate predicate) { } public void setAlwaysFail(KeeperException.Code rc) { - this.alwaysFail.set(rc); + alwaysFail.set(rc); } public void unsetAlwaysFail() { - this.alwaysFail.set(KeeperException.Code.OK); + alwaysFail.set(KeeperException.Code.OK); } public void setSessionId(long id) { sessionId = id; } - @Override - public long getSessionId() { - return sessionId; - } - - private boolean hasChildren(String path) { - return !tree.subMap(path + '/', path + '0').isEmpty(); - } - @Override public String toString() { return "MockZookeeper"; @@ -1182,11 +1166,11 @@ private void checkReadOpDelay() { private void triggerPersistentWatches(String path, String parent, EventType eventType) { persistentWatchers.forEach(w -> { if (w.mode == AddWatchMode.PERSISTENT_RECURSIVE) { - if (path.startsWith(w.getPath())) { + if (path.startsWith(w.path())) { w.watcher.process(new WatchedEvent(eventType, KeeperState.SyncConnected, path)); } } else if (w.mode == AddWatchMode.PERSISTENT) { - if (w.getPath().equals(path)) { + if (w.path().equals(path)) { w.watcher.process(new WatchedEvent(eventType, KeeperState.SyncConnected, path)); } @@ -1199,5 +1183,26 @@ private void triggerPersistentWatches(String path, String parent, EventType even }); } + public void deleteEphemeralNodes(long sessionId) { + if (sessionId != NOT_EPHEMERAL) { + runInExecutorSync(() -> { + tree.values().removeIf(zNode -> zNode.getEphemeralOwner() == sessionId); + }); + } + } + + + public void deleteWatchers(long sessionId) { + runInExecutorSync(() -> { + // remove all persistent watchers for the session + persistentWatchers.removeIf(w -> w.sessionId == sessionId); + // remove all watchers for the session + List> watchersForSession = + watchers.entries().stream().filter(e -> e.getValue().sessionId == sessionId).toList(); + watchersForSession + .forEach(e -> watchers.remove(e.getKey(), e.getValue())); + }); + } + private static final Logger log = LoggerFactory.getLogger(MockZooKeeper.class); } diff --git a/testmocks/src/main/java/org/apache/zookeeper/MockZooKeeperSession.java b/testmocks/src/main/java/org/apache/zookeeper/MockZooKeeperSession.java index a286a75aa9103..c812423b7280d 100644 --- a/testmocks/src/main/java/org/apache/zookeeper/MockZooKeeperSession.java +++ b/testmocks/src/main/java/org/apache/zookeeper/MockZooKeeperSession.java @@ -40,7 +40,7 @@ public class MockZooKeeperSession extends ZooKeeper { private MockZooKeeper mockZooKeeper; - private long sessionId = 0L; + private long sessionId = 1L; private static final Objenesis objenesis = new ObjenesisStd(); @@ -59,6 +59,9 @@ public static MockZooKeeperSession newInstance(MockZooKeeper mockZooKeeper, bool mockZooKeeperSession.mockZooKeeper = mockZooKeeper; mockZooKeeperSession.sessionId = sessionIdGenerator.getAndIncrement(); mockZooKeeperSession.closeMockZooKeeperOnClose = closeMockZooKeeperOnClose; + if (closeMockZooKeeperOnClose) { + mockZooKeeper.increaseRefCount(); + } return mockZooKeeperSession; } @@ -81,17 +84,22 @@ public States getState() { @Override public void register(Watcher watcher) { - mockZooKeeper.register(watcher); + try { + mockZooKeeper.overrideSessionId(getSessionId()); + mockZooKeeper.register(watcher); + } finally { + mockZooKeeper.removeSessionIdOverride(); + } } @Override public String create(String path, byte[] data, List acl, CreateMode createMode) throws KeeperException, InterruptedException { try { - mockZooKeeper.overrideEpheralOwner(getSessionId()); + mockZooKeeper.overrideSessionId(getSessionId()); return mockZooKeeper.create(path, data, acl, createMode); } finally { - mockZooKeeper.removeEpheralOwnerOverride(); + mockZooKeeper.removeSessionIdOverride(); } } @@ -99,134 +107,257 @@ public String create(String path, byte[] data, List acl, CreateMode createM public void create(final String path, final byte[] data, final List acl, CreateMode createMode, final AsyncCallback.StringCallback cb, final Object ctx) { try { - mockZooKeeper.overrideEpheralOwner(getSessionId()); + mockZooKeeper.overrideSessionId(getSessionId()); mockZooKeeper.create(path, data, acl, createMode, cb, ctx); } finally { - mockZooKeeper.removeEpheralOwnerOverride(); + mockZooKeeper.removeSessionIdOverride(); } } @Override - public byte[] getData(String path, Watcher watcher, Stat stat) throws KeeperException { - return mockZooKeeper.getData(path, watcher, stat); + public byte[] getData(String path, Watcher watcher, Stat stat) throws KeeperException, InterruptedException { + try { + mockZooKeeper.overrideSessionId(getSessionId()); + return mockZooKeeper.getData(path, watcher, stat); + } finally { + mockZooKeeper.removeSessionIdOverride(); + } } @Override public void getData(final String path, boolean watch, final DataCallback cb, final Object ctx) { - mockZooKeeper.getData(path, watch, cb, ctx); + try { + mockZooKeeper.overrideSessionId(getSessionId()); + mockZooKeeper.getData(path, watch, cb, ctx); + } finally { + mockZooKeeper.removeSessionIdOverride(); + } } @Override public void getData(final String path, final Watcher watcher, final DataCallback cb, final Object ctx) { - mockZooKeeper.getData(path, watcher, cb, ctx); + try { + mockZooKeeper.overrideSessionId(getSessionId()); + mockZooKeeper.getData(path, watcher, cb, ctx); + } finally { + mockZooKeeper.removeSessionIdOverride(); + } } @Override public void getChildren(final String path, final Watcher watcher, final ChildrenCallback cb, final Object ctx) { - mockZooKeeper.getChildren(path, watcher, cb, ctx); + try { + mockZooKeeper.overrideSessionId(getSessionId()); + mockZooKeeper.getChildren(path, watcher, cb, ctx); + } finally { + mockZooKeeper.removeSessionIdOverride(); + } } @Override - public List getChildren(String path, Watcher watcher) throws KeeperException { - return mockZooKeeper.getChildren(path, watcher); + public List getChildren(String path, Watcher watcher) throws KeeperException, InterruptedException { + try { + mockZooKeeper.overrideSessionId(getSessionId()); + return mockZooKeeper.getChildren(path, watcher); + } finally { + mockZooKeeper.removeSessionIdOverride(); + } } @Override public List getChildren(String path, boolean watch) throws KeeperException, InterruptedException { - return mockZooKeeper.getChildren(path, watch); + try { + mockZooKeeper.overrideSessionId(getSessionId()); + return mockZooKeeper.getChildren(path, watch); + } finally { + mockZooKeeper.removeSessionIdOverride(); + } } @Override public void getChildren(final String path, boolean watcher, final Children2Callback cb, final Object ctx) { - mockZooKeeper.getChildren(path, watcher, cb, ctx); + try { + mockZooKeeper.overrideSessionId(getSessionId()); + mockZooKeeper.getChildren(path, watcher, cb, ctx); + } finally { + mockZooKeeper.removeSessionIdOverride(); + } } @Override public Stat exists(String path, boolean watch) throws KeeperException, InterruptedException { - return mockZooKeeper.exists(path, watch); + try { + mockZooKeeper.overrideSessionId(getSessionId()); + return mockZooKeeper.exists(path, watch); + } finally { + mockZooKeeper.removeSessionIdOverride(); + } } @Override public Stat exists(String path, Watcher watcher) throws KeeperException, InterruptedException { - return mockZooKeeper.exists(path, watcher); + try { + mockZooKeeper.overrideSessionId(getSessionId()); + return mockZooKeeper.exists(path, watcher); + } finally { + mockZooKeeper.removeSessionIdOverride(); + } } @Override public void exists(String path, boolean watch, StatCallback cb, Object ctx) { - mockZooKeeper.exists(path, watch, cb, ctx); + try { + mockZooKeeper.overrideSessionId(getSessionId()); + mockZooKeeper.exists(path, watch, cb, ctx); + } finally { + mockZooKeeper.removeSessionIdOverride(); + } } @Override public void exists(String path, Watcher watcher, StatCallback cb, Object ctx) { - mockZooKeeper.exists(path, watcher, cb, ctx); + try { + mockZooKeeper.overrideSessionId(getSessionId()); + mockZooKeeper.exists(path, watcher, cb, ctx); + } finally { + mockZooKeeper.removeSessionIdOverride(); + } } @Override public void sync(String path, VoidCallback cb, Object ctx) { - mockZooKeeper.sync(path, cb, ctx); + try { + mockZooKeeper.overrideSessionId(getSessionId()); + mockZooKeeper.sync(path, cb, ctx); + } finally { + mockZooKeeper.removeSessionIdOverride(); + } } @Override public Stat setData(final String path, byte[] data, int version) throws KeeperException, InterruptedException { - return mockZooKeeper.setData(path, data, version); + try { + mockZooKeeper.overrideSessionId(getSessionId()); + return mockZooKeeper.setData(path, data, version); + } finally { + mockZooKeeper.removeSessionIdOverride(); + } } @Override public void setData(final String path, final byte[] data, int version, final StatCallback cb, final Object ctx) { - mockZooKeeper.setData(path, data, version, cb, ctx); + try { + mockZooKeeper.overrideSessionId(getSessionId()); + mockZooKeeper.setData(path, data, version, cb, ctx); + } finally { + mockZooKeeper.removeSessionIdOverride(); + } } @Override public void delete(final String path, int version) throws InterruptedException, KeeperException { - mockZooKeeper.delete(path, version); + try { + mockZooKeeper.overrideSessionId(getSessionId()); + mockZooKeeper.delete(path, version); + } finally { + mockZooKeeper.removeSessionIdOverride(); + } } @Override public void delete(final String path, int version, final VoidCallback cb, final Object ctx) { - mockZooKeeper.delete(path, version, cb, ctx); + try { + mockZooKeeper.overrideSessionId(getSessionId()); + mockZooKeeper.delete(path, version, cb, ctx); + } finally { + mockZooKeeper.removeSessionIdOverride(); + } } @Override public void multi(Iterable ops, AsyncCallback.MultiCallback cb, Object ctx) { - mockZooKeeper.multi(ops, cb, ctx); + try { + mockZooKeeper.overrideSessionId(getSessionId()); + mockZooKeeper.multi(ops, cb, ctx); + } finally { + mockZooKeeper.removeSessionIdOverride(); + } } @Override public List multi(Iterable ops) throws InterruptedException, KeeperException { - return mockZooKeeper.multi(ops); + try { + mockZooKeeper.overrideSessionId(getSessionId()); + return mockZooKeeper.multi(ops); + } finally { + mockZooKeeper.removeSessionIdOverride(); + } } @Override public void addWatch(String basePath, Watcher watcher, AddWatchMode mode, VoidCallback cb, Object ctx) { - mockZooKeeper.addWatch(basePath, watcher, mode, cb, ctx); + try { + mockZooKeeper.overrideSessionId(getSessionId()); + mockZooKeeper.addWatch(basePath, watcher, mode, cb, ctx); + } finally { + mockZooKeeper.removeSessionIdOverride(); + } } @Override public void addWatch(String basePath, Watcher watcher, AddWatchMode mode) throws KeeperException, InterruptedException { - mockZooKeeper.addWatch(basePath, watcher, mode); + try { + mockZooKeeper.overrideSessionId(getSessionId()); + mockZooKeeper.addWatch(basePath, watcher, mode); + } finally { + mockZooKeeper.removeSessionIdOverride(); + } } @Override public void addWatch(String basePath, AddWatchMode mode) throws KeeperException, InterruptedException { - mockZooKeeper.addWatch(basePath, mode); + try { + mockZooKeeper.overrideSessionId(getSessionId()); + mockZooKeeper.addWatch(basePath, mode); + } finally { + mockZooKeeper.removeSessionIdOverride(); + } } @Override public void addWatch(String basePath, AddWatchMode mode, VoidCallback cb, Object ctx) { - mockZooKeeper.addWatch(basePath, mode, cb, ctx); + try { + mockZooKeeper.overrideSessionId(getSessionId()); + mockZooKeeper.addWatch(basePath, mode, cb, ctx); + } finally { + mockZooKeeper.removeSessionIdOverride(); + } } @Override public void close() throws InterruptedException { - if (closeMockZooKeeperOnClose) { - mockZooKeeper.close(); - } + internalClose(false); } public void shutdown() throws InterruptedException { - if (closeMockZooKeeperOnClose) { - mockZooKeeper.shutdown(); + internalClose(true); + } + + private void internalClose(boolean shutdown) throws InterruptedException { + try { + mockZooKeeper.overrideSessionId(getSessionId()); + mockZooKeeper.deleteEphemeralNodes(getSessionId()); + mockZooKeeper.deleteWatchers(getSessionId()); + if (closeMockZooKeeperOnClose) { + if (shutdown) { + mockZooKeeper.shutdown(); + } else { + mockZooKeeper.close(); + } + } + } finally { + mockZooKeeper.removeSessionIdOverride(); } } From bcf8afb991d67f82eca55f76a046c86391364a47 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Mon, 17 Feb 2025 22:22:06 -0800 Subject: [PATCH 15/16] [fix][test] Fix flaky test MetadataStoreTest.emptyStoreTest (#23998) --- .../test/java/org/apache/pulsar/metadata/MetadataStoreTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataStoreTest.java b/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataStoreTest.java index a7b1dcf6bf02b..7a333391dc9eb 100644 --- a/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataStoreTest.java +++ b/pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataStoreTest.java @@ -114,7 +114,7 @@ public void concurrentPutTest(String provider, Supplier urlSupplier) thr MetadataStoreConfig.builder().fsyncEnable(false).build()); String data = "data"; - String path = "/non-existing-key"; + String path = "/concurrentPutTest"; int concurrent = 50; List> futureList = new ArrayList<>(); for (int i = 0; i < concurrent; i++) { From 4bfdcd85ff5c9f21ba4fd976adf7785465053151 Mon Sep 17 00:00:00 2001 From: Lari Hotari Date: Mon, 17 Feb 2025 23:11:26 -0800 Subject: [PATCH 16/16] [improve][meta] Simplify getting parent path in ZKMetadataStore without using java.io.File (#23996) --- .../org/apache/pulsar/metadata/impl/ZKMetadataStore.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/ZKMetadataStore.java b/pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/ZKMetadataStore.java index dab3553f8ce1b..56c0962f2504f 100644 --- a/pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/ZKMetadataStore.java +++ b/pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/ZKMetadataStore.java @@ -19,7 +19,6 @@ package org.apache.pulsar.metadata.impl; import com.google.common.annotations.VisibleForTesting; -import java.io.File; import java.util.Collections; import java.util.EnumSet; import java.util.List; @@ -610,8 +609,10 @@ private static void asyncCreateFullPathOptimistic(final ZooKeeper zk, final Stri if (rc != Code.NONODE.intValue()) { callback.processResult(rc, path, ctx, name); } else { - String parent = (new File(originalPath)).getParent().replace("\\", "/"); - + String parent = parent(originalPath); + if (parent == null) { + parent = "/"; + } // Create parent nodes as "CONTAINER" so that ZK will automatically delete them when they're empty asyncCreateFullPathOptimistic(zk, parent, new byte[0], CreateMode.CONTAINER, (rc1, path1, ctx1, name1) -> {