You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/12/01 10:14:14 UTC

[GitHub] [pulsar] codelipenghui opened a new pull request, #18701: [fix][broker] Fix duplicated schemas creation

codelipenghui opened a new pull request, #18701:
URL: https://github.com/apache/pulsar/pull/18701

   Fixes: #18292
   Another solution of #18293
   
   ### Motivation
   
   Fix the duplicated schemas creation due to race conditions.
   The broker will get all the schemas to check whether or not the given schema exists.
   And then put the schema into the schema storage once the schema doesn't exist.
   But If there are parallel requests with the same schema. 
   The Schema registry will put duplicated same schemas into the
   schema storage.
   
   For each schema update, we need to update the schema locator.
   The PR uses the Zookeeper data version control to ensure
   the schema locator will not be updated based on outdated data.
   And will retry to check if schema exists and compatibility while
   encountering bad version exceptions.
   
   And the created Ledger with failed locator updating will be removed
   to avoid the Ledger leak issue.
   
   ### Modifications
   
   Add a new put schema API, which can accept a function to check if the
   schema exists or is compatible.
   
   ### Verifying this change
   
   A new test was added for creating producers with schema in parallel
   
   ### Documentation
   
   <!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
   
   - [ ] `doc` <!-- Your PR contains doc changes. Please attach the local preview screenshots (run `sh start.sh` at `pulsar/site2/website`) to your PR description, or else your PR might not get merged. -->
   - [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
   - [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
   - [ ] `doc-complete` <!-- Docs have been already added -->
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] liangyepianzhou commented on a diff in pull request #18701: [fix][broker] Fix duplicated schemas creation

Posted by GitBox <gi...@apache.org>.
liangyepianzhou commented on code in PR #18701:
URL: https://github.com/apache/pulsar/pull/18701#discussion_r1039074901


##########
pulsar-common/src/main/java/org/apache/pulsar/common/protocol/schema/SchemaStorage.java:
##########
@@ -28,6 +30,20 @@ public interface SchemaStorage {
 
     CompletableFuture<SchemaVersion> put(String key, byte[] value, byte[] hash);
 
+    /**
+     * Put the schema to the schema storage.
+     *
+     * @param key The schema ID
+     * @param fn The function to calculate the value and hash that need to put to the schema storage
+     *           The input of the function is all the existing schemas that used to do the schemas compatibility check
+     * @return The schema version of the stored schema
+     */
+    default CompletableFuture<SchemaVersion> put(String key,

Review Comment:
   LGTM! But I have a question why do we need to add a default method when the interface only has one implementation class?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] codecov-commenter commented on pull request #18701: [fix][broker] Fix duplicated schemas creation

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #18701:
URL: https://github.com/apache/pulsar/pull/18701#issuecomment-1333568155

   # [Codecov](https://codecov.io/gh/apache/pulsar/pull/18701?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#18701](https://codecov.io/gh/apache/pulsar/pull/18701?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9a305b3) into [master](https://codecov.io/gh/apache/pulsar/commit/90c5534cd75275613a014640ea6283afdf1721d3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (90c5534) will **decrease** coverage by `12.98%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/pulsar/pull/18701/graphs/tree.svg?width=650&height=150&src=pr&token=acYqCpsK9J&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/pulsar/pull/18701?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@              Coverage Diff              @@
   ##             master   #18701       +/-   ##
   =============================================
   - Coverage     50.05%   37.07%   -12.99%     
   + Complexity     9706     1967     -7739     
   =============================================
     Files           618      209      -409     
     Lines         58586    14421    -44165     
     Branches       6098     1573     -4525     
   =============================================
   - Hits          29327     5347    -23980     
   + Misses        26092     8487    -17605     
   + Partials       3167      587     -2580     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | unittests | `37.07% <ø> (-12.99%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/pulsar/pull/18701?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...g/apache/pulsar/client/impl/ConnectionHandler.java](https://codecov.io/gh/apache/pulsar/pull/18701/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL0Nvbm5lY3Rpb25IYW5kbGVyLmphdmE=) | `50.00% <0.00%> (-5.32%)` | :arrow_down: |
   | [.../org/apache/pulsar/client/impl/ConnectionPool.java](https://codecov.io/gh/apache/pulsar/pull/18701/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL0Nvbm5lY3Rpb25Qb29sLmphdmE=) | `37.43% <0.00%> (-1.03%)` | :arrow_down: |
   | [...apache/pulsar/client/impl/AutoClusterFailover.java](https://codecov.io/gh/apache/pulsar/pull/18701/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL0F1dG9DbHVzdGVyRmFpbG92ZXIuamF2YQ==) | `69.44% <0.00%> (-0.56%)` | :arrow_down: |
   | [...broker/service/schema/BookkeeperSchemaStorage.java](https://codecov.io/gh/apache/pulsar/pull/18701/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL3NjaGVtYS9Cb29ra2VlcGVyU2NoZW1hU3RvcmFnZS5qYXZh) | | |
   | [...oker/service/schema/SchemaRegistryServiceImpl.java](https://codecov.io/gh/apache/pulsar/pull/18701/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL3NjaGVtYS9TY2hlbWFSZWdpc3RyeVNlcnZpY2VJbXBsLmphdmE=) | | |
   | [...broker/loadbalance/ModularLoadManagerStrategy.java](https://codecov.io/gh/apache/pulsar/pull/18701/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9sb2FkYmFsYW5jZS9Nb2R1bGFyTG9hZE1hbmFnZXJTdHJhdGVneS5qYXZh) | | |
   | [...oker/stats/prometheus/PrometheusMetricStreams.java](https://codecov.io/gh/apache/pulsar/pull/18701/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zdGF0cy9wcm9tZXRoZXVzL1Byb21ldGhldXNNZXRyaWNTdHJlYW1zLmphdmE=) | | |
   | [...ion/pendingack/impl/MLPendingAckStoreProvider.java](https://codecov.io/gh/apache/pulsar/pull/18701/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci90cmFuc2FjdGlvbi9wZW5kaW5nYWNrL2ltcGwvTUxQZW5kaW5nQWNrU3RvcmVQcm92aWRlci5qYXZh) | | |
   | [...r/service/schema/AvroSchemaCompatibilityCheck.java](https://codecov.io/gh/apache/pulsar/pull/18701/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL3NjaGVtYS9BdnJvU2NoZW1hQ29tcGF0aWJpbGl0eUNoZWNrLmphdmE=) | | |
   | [...s/prometheus/metrics/PrometheusTextFormatUtil.java](https://codecov.io/gh/apache/pulsar/pull/18701/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zdGF0cy9wcm9tZXRoZXVzL21ldHJpY3MvUHJvbWV0aGV1c1RleHRGb3JtYXRVdGlsLmphdmE=) | | |
   | ... and [402 more](https://codecov.io/gh/apache/pulsar/pull/18701/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] Technoboy- merged pull request #18701: [fix][broker] Fix duplicated schemas creation

Posted by GitBox <gi...@apache.org>.
Technoboy- merged PR #18701:
URL: https://github.com/apache/pulsar/pull/18701


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] codelipenghui commented on a diff in pull request #18701: [fix][broker] Fix duplicated schemas creation

Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #18701:
URL: https://github.com/apache/pulsar/pull/18701#discussion_r1039119106


##########
pulsar-common/src/main/java/org/apache/pulsar/common/protocol/schema/SchemaStorage.java:
##########
@@ -28,6 +30,20 @@ public interface SchemaStorage {
 
     CompletableFuture<SchemaVersion> put(String key, byte[] value, byte[] hash);
 
+    /**
+     * Put the schema to the schema storage.
+     *
+     * @param key The schema ID
+     * @param fn The function to calculate the value and hash that need to put to the schema storage
+     *           The input of the function is all the existing schemas that used to do the schemas compatibility check
+     * @return The schema version of the stored schema
+     */
+    default CompletableFuture<SchemaVersion> put(String key,

Review Comment:
   @liangyepianzhou 
   
   Users can have their own schema registry implementation 
   
   Please see the broker.conf
   
   ```
   ### --- Schema storage --- ###
   # The schema storage implementation used by this broker
   schemaRegistryStorageClassName=org.apache.pulsar.broker.service.schema.BookkeeperSchemaStorageFactory
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org