aloyszhang commented on code in PR #18293:
URL: https://github.com/apache/pulsar/pull/18293#discussion_r1015195639
##########
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.
`this.stats.recordPutLatenc` is recorded incorrectly since `long start =
this.clock.millis()` , I'll fix this;
`this.stats.recordPutFailed` has been recorded when exceptions (not
`AlreadyExistsException` or `BadVersionException`) happened, so no need to
record again.
> Maybe we can add a new method putSchemaWithRetryIfAbsent, this new method
only wraps the original method putSchemaIfAbsent
Agree, and add a new metric to indicate `putOpsRetryCounter` retry times
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
