Skip to content
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

Schema registry 3/N #1363

Merged
merged 79 commits into from Mar 14, 2018
Merged

Schema registry 3/N #1363

merged 79 commits into from Mar 14, 2018

Conversation

mgodave
Copy link
Contributor

@mgodave mgodave commented Mar 9, 2018

See #1137 for reference

Dave Rusek added 30 commits February 12, 2018 18:09
# Conflicts:
#	pulsar-broker/src/main/java/org/apache/pulsar/broker/service/nonpersistent/NonPersistentTopic.java
#	pulsar-common/src/main/java/org/apache/pulsar/common/api/Commands.java
Dave Rusek added 18 commits March 2, 2018 14:10
# Conflicts:
#	pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
#	pulsar-common/pom.xml
# Conflicts:
#	pulsar-broker/pom.xml
#	pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/SchemaRegistryServiceImpl.java
#	pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/SchemaStorage.java
#	pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/StoredSchema.java
#	pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/proto/SchemaRegistryFormat.java
#	pulsar-common/src/main/java/org/apache/pulsar/common/schema/EmptyVersion.java
#	pulsar-common/src/main/java/org/apache/pulsar/common/schema/SchemaData.java
Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
* <p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The <p> is failing the license check

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, just re-ran the formatter.

@VisibleForTesting
public void init() throws KeeperException, InterruptedException {
try {
zooKeeper.create(SchemaPath, new byte[]{}, Acl, CreateMode.PERSISTENT);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every time we do this it will actually do the write the operation on journal and then log something in zk server that it failed.

If it's very unlikely that the condition is true (just the very first time), we do and exists() call to avoid the write.

@Override
public SchemaVersion versionFromBytes(byte[] version) {
ByteBuffer bb = ByteBuffer.wrap(version);
return new LongSchemaVersion(bb.get());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get() is just reading the first byte here and ignoring the remaining 7

Copy link
Contributor Author

@mgodave mgodave Mar 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. Good catch. it was a type it should have been getLong() but I guess the tests pass since the version numbers tend to be low and fit within the first byte.

Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@merlimat merlimat merged commit 80dc0de into apache:master Mar 14, 2018
@mgodave mgodave deleted the schema-registry-3 branch March 14, 2018 15:28
sijie pushed a commit to sijie/pulsar that referenced this pull request Aug 26, 2020
gaoran10 pushed a commit to gaoran10/pulsar that referenced this pull request Sep 17, 2020
zymap pushed a commit to zymap/pulsar that referenced this pull request Nov 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants