poorbarcode commented on code in PR #18293:
URL: https://github.com/apache/pulsar/pull/18293#discussion_r1016075114
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/SchemaRegistryServiceImpl.java:
##########
@@ -180,57 +182,85 @@ public
CompletableFuture<List<CompletableFuture<SchemaAndMetadata>>> getAllSchem
@Override
@NotNull
public CompletableFuture<SchemaVersion> putSchemaIfAbsent(String schemaId,
SchemaData schema,
-
SchemaCompatibilityStrategy strategy) {
+ SchemaCompatibilityStrategy strategy) {
+
+ return putSchemaIfAbsent(schemaId, schema, strategy, 0, 0);
+ }
+
+ @NotNull
+ public CompletableFuture<SchemaVersion> putSchemaIfAbsent(String schemaId,
SchemaData schema,
+ SchemaCompatibilityStrategy strategy, long startTime, int retry) {
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);
- }
- });
-
- });
-
- }));
+ 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 = startTime > 0 ? startTime :
this.clock.millis();
+ CompletableFuture<SchemaVersion> persistentFuture =
new CompletableFuture<>();
+ schemaStorage
Review Comment:
I see the current code is that when finds `BadVersion` then tries again
until it succeeds, maybe we can rewrite it like this:
```java
long start = startTime > 0 ? startTime : this.clock.millis();
return schemaStorage.put(schemaId, info.toByteArray(),
context).handleAsync((sv, ex) -> {
if (ex != null){
return CompletableFuture.completedFuture(sv);
}
if (xx instanceof xxx){
return putSchemaIfAbsent(schemaId, schema, strategy, start, retryTime +
1);
}
return CompletableFuture.failedFuture(ex);
});
```
this makes it easier to read.
BTW, This change does not seem to solve the problem to be solved
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/SchemaRegistryServiceImpl.java:
##########
@@ -180,57 +182,85 @@ public
CompletableFuture<List<CompletableFuture<SchemaAndMetadata>>> getAllSchem
@Override
@NotNull
public CompletableFuture<SchemaVersion> putSchemaIfAbsent(String schemaId,
SchemaData schema,
-
SchemaCompatibilityStrategy strategy) {
+ SchemaCompatibilityStrategy strategy) {
+
+ return putSchemaIfAbsent(schemaId, schema, strategy, 0, 0);
+ }
+
+ @NotNull
+ public CompletableFuture<SchemaVersion> putSchemaIfAbsent(String schemaId,
SchemaData schema,
+ SchemaCompatibilityStrategy strategy, long startTime, int retry) {
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);
- }
- });
-
- });
-
- }));
+ 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 = startTime > 0 ? startTime :
this.clock.millis();
+ CompletableFuture<SchemaVersion> persistentFuture =
new CompletableFuture<>();
+ 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.getCause() instanceof
AlreadyExistsException
+ || t.getCause() instanceof
BadVersionException) {
+ // retry if put schemaLocator to zk
failed caused by race condition
+ int retryTime = retry > 0 ? retry + 1
: 1;
Review Comment:
seems `retry` always `>=0`, maybe we can write this line like this: `int
retryTime = retry+1`
--
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]
