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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
