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/11/07 07:32:32 UTC
[GitHub] [pulsar] poorbarcode commented on a diff in pull request #18293: [fix][broker][schema]fix creating lots of ledgers for the same schema
poorbarcode commented on code in PR #18293:
URL: https://github.com/apache/pulsar/pull/18293#discussion_r1015080057
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/SchemaRegistryServiceImpl.java:
##########
@@ -181,56 +183,85 @@ public CompletableFuture<List<CompletableFuture<SchemaAndMetadata>>> getAllSchem
@NotNull
public CompletableFuture<SchemaVersion> putSchemaIfAbsent(String schemaId, SchemaData schema,
SchemaCompatibilityStrategy strategy) {
- return trimDeletedSchemaAndGetList(schemaId).thenCompose(schemaAndMetadataList ->
- getSchemaVersionBySchemaData(schemaAndMetadataList, schema).thenCompose(schemaVersion -> {
- if (schemaVersion != null) {
- if (log.isDebugEnabled()) {
- log.debug("[{}] Schema is already exists", schemaId);
- }
- return CompletableFuture.completedFuture(schemaVersion);
- }
- CompletableFuture<Void> checkCompatibilityFuture = new CompletableFuture<>();
- if (schemaAndMetadataList.size() != 0) {
- if (isTransitiveStrategy(strategy)) {
- checkCompatibilityFuture =
- checkCompatibilityWithAll(schemaId, schema, strategy, schemaAndMetadataList);
- } else {
- checkCompatibilityFuture = checkCompatibilityWithLatest(schemaId, schema, strategy);
- }
- } else {
- checkCompatibilityFuture.complete(null);
- }
- return checkCompatibilityFuture.thenCompose(v -> {
- byte[] context = hashFunction.hashBytes(schema.getData()).asBytes();
- SchemaRegistryFormat.SchemaInfo info = SchemaRegistryFormat.SchemaInfo.newBuilder()
- .setType(Functions.convertFromDomainType(schema.getType()))
- .setSchema(ByteString.copyFrom(schema.getData()))
- .setSchemaId(schemaId)
- .setUser(schema.getUser())
- .setDeleted(false)
- .setTimestamp(clock.millis())
- .addAllProps(toPairs(schema.getProps()))
- .build();
-
- long start = this.clock.millis();
-
- return schemaStorage
- .put(schemaId, info.toByteArray(), context)
- .whenComplete((__, t) -> {
- if (t != null) {
- log.error("[{}] Put schema failed", schemaId);
- this.stats.recordPutFailed(schemaId);
- } else {
- if (log.isDebugEnabled()) {
- log.debug("[{}] Put schema finished", schemaId);
- }
- this.stats.recordPutLatency(schemaId, this.clock.millis() - start);
- }
- });
-
- });
-
- }));
+ CompletableFuture<SchemaVersion> persistentFuture = new CompletableFuture<>();
+ return trimDeletedSchemaAndGetList(schemaId)
+ .thenCompose(schemaAndMetadataList ->
+ getSchemaVersionBySchemaData(schemaAndMetadataList, schema)
+ .thenCompose(schemaVersion -> {
+ if (schemaVersion != null) {
+ if (log.isDebugEnabled()) {
+ log.debug("[{}] Schema is already exists", schemaId);
+ }
+ return CompletableFuture.completedFuture(schemaVersion);
+ }
+ CompletableFuture<Void> checkCompatibilityFuture = new CompletableFuture<>();
+ if (schemaAndMetadataList.size() != 0) {
+ if (isTransitiveStrategy(strategy)) {
+ checkCompatibilityFuture = checkCompatibilityWithAll(schemaId, schema,
+ strategy, schemaAndMetadataList);
+ } else {
+ checkCompatibilityFuture = checkCompatibilityWithLatest(schemaId,
+ schema, strategy);
+ }
+ } else {
+ checkCompatibilityFuture.complete(null);
+ }
+ return checkCompatibilityFuture.thenCompose(v -> {
+ byte[] context = hashFunction.hashBytes(schema.getData()).asBytes();
+ SchemaRegistryFormat.SchemaInfo info = SchemaRegistryFormat.SchemaInfo
+ .newBuilder()
+ .setType(Functions.convertFromDomainType(schema.getType()))
+ .setSchema(ByteString.copyFrom(schema.getData()))
+ .setSchemaId(schemaId)
+ .setUser(schema.getUser())
+ .setDeleted(false)
+ .setTimestamp(clock.millis())
+ .addAllProps(toPairs(schema.getProps()))
+ .build();
+
+ long start = this.clock.millis();
+ schemaStorage
+ .put(schemaId, info.toByteArray(), context)
+ .thenAccept(sv -> {
+ if (log.isDebugEnabled()) {
+ log.debug("[{}] Schema is successfully added", schemaId);
+ }
+ this.stats.recordPutLatency(schemaId, this.clock.millis() - start);
+ persistentFuture.complete(sv);
+ })
+ .exceptionally(t -> {
+ if (t != null) {
+ if (t.getCause() instanceof AlreadyExistsException
+ || t.getCause() instanceof BadVersionException) {
+ // retry if put schemaLocator to zk failed caused by
+ // race condition
+ if (log.isDebugEnabled()) {
+ log.debug("[{}] Put schema failed because of {}, retry",
+ t.getCause().getMessage(), schemaId);
+ }
+ putSchemaIfAbsent(schemaId, schema, strategy)
Review Comment:
Call `putSchemaIfAbsent` here will retry once, but it will not record metrics `this.stats.recordPutLatency` or `this.stats.recordPutFailed`. Maybe we can add a new method `putSchemaWithRetryIfAbsent`, this new method only wraps the original method `putSchemaIfAbsent`
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/SchemaRegistryServiceImpl.java:
##########
@@ -181,56 +183,85 @@ public CompletableFuture<List<CompletableFuture<SchemaAndMetadata>>> getAllSchem
@NotNull
public CompletableFuture<SchemaVersion> putSchemaIfAbsent(String schemaId, SchemaData schema,
SchemaCompatibilityStrategy strategy) {
- return trimDeletedSchemaAndGetList(schemaId).thenCompose(schemaAndMetadataList ->
- getSchemaVersionBySchemaData(schemaAndMetadataList, schema).thenCompose(schemaVersion -> {
- if (schemaVersion != null) {
- if (log.isDebugEnabled()) {
- log.debug("[{}] Schema is already exists", schemaId);
- }
- return CompletableFuture.completedFuture(schemaVersion);
- }
- CompletableFuture<Void> checkCompatibilityFuture = new CompletableFuture<>();
- if (schemaAndMetadataList.size() != 0) {
- if (isTransitiveStrategy(strategy)) {
- checkCompatibilityFuture =
- checkCompatibilityWithAll(schemaId, schema, strategy, schemaAndMetadataList);
- } else {
- checkCompatibilityFuture = checkCompatibilityWithLatest(schemaId, schema, strategy);
- }
- } else {
- checkCompatibilityFuture.complete(null);
- }
- return checkCompatibilityFuture.thenCompose(v -> {
- byte[] context = hashFunction.hashBytes(schema.getData()).asBytes();
- SchemaRegistryFormat.SchemaInfo info = SchemaRegistryFormat.SchemaInfo.newBuilder()
- .setType(Functions.convertFromDomainType(schema.getType()))
- .setSchema(ByteString.copyFrom(schema.getData()))
- .setSchemaId(schemaId)
- .setUser(schema.getUser())
- .setDeleted(false)
- .setTimestamp(clock.millis())
- .addAllProps(toPairs(schema.getProps()))
- .build();
-
- long start = this.clock.millis();
-
- return schemaStorage
- .put(schemaId, info.toByteArray(), context)
- .whenComplete((__, t) -> {
- if (t != null) {
- log.error("[{}] Put schema failed", schemaId);
- this.stats.recordPutFailed(schemaId);
- } else {
- if (log.isDebugEnabled()) {
- log.debug("[{}] Put schema finished", schemaId);
- }
- this.stats.recordPutLatency(schemaId, this.clock.millis() - start);
- }
- });
-
- });
-
- }));
+ CompletableFuture<SchemaVersion> persistentFuture = new CompletableFuture<>();
+ return trimDeletedSchemaAndGetList(schemaId)
+ .thenCompose(schemaAndMetadataList ->
+ getSchemaVersionBySchemaData(schemaAndMetadataList, schema)
+ .thenCompose(schemaVersion -> {
+ if (schemaVersion != null) {
+ if (log.isDebugEnabled()) {
+ log.debug("[{}] Schema is already exists", schemaId);
+ }
+ return CompletableFuture.completedFuture(schemaVersion);
+ }
+ CompletableFuture<Void> checkCompatibilityFuture = new CompletableFuture<>();
+ if (schemaAndMetadataList.size() != 0) {
+ if (isTransitiveStrategy(strategy)) {
+ checkCompatibilityFuture = checkCompatibilityWithAll(schemaId, schema,
+ strategy, schemaAndMetadataList);
+ } else {
+ checkCompatibilityFuture = checkCompatibilityWithLatest(schemaId,
+ schema, strategy);
+ }
+ } else {
+ checkCompatibilityFuture.complete(null);
+ }
+ return checkCompatibilityFuture.thenCompose(v -> {
+ byte[] context = hashFunction.hashBytes(schema.getData()).asBytes();
+ SchemaRegistryFormat.SchemaInfo info = SchemaRegistryFormat.SchemaInfo
+ .newBuilder()
+ .setType(Functions.convertFromDomainType(schema.getType()))
+ .setSchema(ByteString.copyFrom(schema.getData()))
+ .setSchemaId(schemaId)
+ .setUser(schema.getUser())
+ .setDeleted(false)
+ .setTimestamp(clock.millis())
+ .addAllProps(toPairs(schema.getProps()))
+ .build();
+
+ long start = this.clock.millis();
+ schemaStorage
+ .put(schemaId, info.toByteArray(), context)
+ .thenAccept(sv -> {
+ if (log.isDebugEnabled()) {
+ log.debug("[{}] Schema is successfully added", schemaId);
+ }
+ this.stats.recordPutLatency(schemaId, this.clock.millis() - start);
+ persistentFuture.complete(sv);
+ })
+ .exceptionally(t -> {
+ if (t != null) {
Review Comment:
We do not need these two logic branch, right?
```java
.exceptionally(t ->{
// the variable t will not be null
}
```
--
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