-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Global schema #45
Global schema #45
Conversation
…dded the ability to assign globally unique schema id on the master and retrieve schemas using ids 3. Unit tests need to be modified Conflicts: config/schema-registry.properties src/main/java/io/confluent/kafka/schemaregistry/rest/Main.java src/main/java/io/confluent/kafka/schemaregistry/rest/RegisterSchemaForwardingAgent.java src/main/java/io/confluent/kafka/schemaregistry/rest/SchemaRegistryConfig.java src/main/java/io/confluent/kafka/schemaregistry/rest/SchemaRegistryRestApplication.java src/main/java/io/confluent/kafka/schemaregistry/rest/entities/requests/RegisterSchemaResponse.java src/main/java/io/confluent/kafka/schemaregistry/rest/resources/SchemasResource.java src/main/java/io/confluent/kafka/schemaregistry/rest/resources/SubjectsResource.java src/main/java/io/confluent/kafka/schemaregistry/storage/KafkaSchemaRegistry.java src/main/java/io/confluent/kafka/schemaregistry/storage/KafkaStore.java src/main/java/io/confluent/kafka/schemaregistry/storage/SchemaRegistry.java src/main/java/io/confluent/kafka/schemaregistry/utils/RestUtils.java src/test/java/io/confluent/kafka/schemaregistry/RestApp.java src/test/java/io/confluent/kafka/schemaregistry/storage/StringMessageHandler.java src/test/java/io/confluent/kafka/schemaregistry/utils/TestUtils.java
…ves to be too memory intensive during performance testing, we can change it to a memory mapped index constructed from scratch during bootstrap 2. All tests pass. Changed tests to validate schemas based on ids. Also included tests that get the same schema using id and subject,version Conflicts: src/main/java/io/confluent/kafka/schemaregistry/storage/KafkaSchemaRegistry.java src/main/java/io/confluent/kafka/schemaregistry/storage/KafkaStore.java src/main/java/io/confluent/kafka/schemaregistry/storage/KafkaStoreMessageHandler.java src/main/java/io/confluent/kafka/schemaregistry/storage/KafkaStoreReaderThread.java src/main/java/io/confluent/kafka/schemaregistry/storage/StoreUpdateHandler.java src/main/java/io/confluent/kafka/schemaregistry/utils/RestUtils.java src/test/java/io/confluent/kafka/schemaregistry/rest/RestApiTest.java src/test/java/io/confluent/kafka/schemaregistry/storage/StoreUtils.java src/test/java/io/confluent/kafka/schemaregistry/utils/TestUtils.java src/test/java/io/confluent/kafka/schemaregistry/zookeeper/MasterElectorTest.java
…s not include re-registering schemas. There are 2 test failures related to that. It will work when we add that capability
…s is currently unused).
public static final int MIN_VERSION = 1; | ||
public static final int MAX_VERSION = Integer.MAX_VALUE; | ||
private static final String ZOOKEEPER_SCHEMA_ID_COUNTER = "/schema_id_counter"; | ||
private static final int ZOOKEEPER_SCHEMA_ID_COUNTER_BATCH_SIZE = 1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, use smaller counter batch size.
@@ -35,6 +37,23 @@ public SubjectsResource(SchemaRegistry schemaRegistry) { | |||
this.schemaRegistry = schemaRegistry; | |||
} | |||
|
|||
@GET | |||
@Path("/{id}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nehanarkhede Just double-checking - didn't we want this path to be /schemas/{id} rather than /subjects/{id}?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@granders Good catch. Yes, it makes more sense as /schemas/{id}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be addressed in #48
@nehanarkhede This looks good to me overall, let me know if you think it's ready for a merge. |
@granders Let's merge. |
No description provided.