[GitHub] nifi pull request #2523: NIFI-4935 Support Schema Branches when using HWX Sc...
Github user asfgit closed the pull request at: https://github.com/apache/nifi/pull/2523 ---
[GitHub] nifi pull request #2523: NIFI-4935 Support Schema Branches when using HWX Sc...
Github user bbende commented on a diff in the pull request: https://github.com/apache/nifi/pull/2523#discussion_r174895925 --- Diff: nifi-nar-bundles/nifi-extension-utils/nifi-record-utils/nifi-standard-record-utils/src/main/java/org/apache/nifi/schema/access/HortonworksAttributeSchemaReferenceWriter.java --- @@ -40,8 +40,8 @@ public void writeHeader(RecordSchema schema, OutputStream out) throws IOExceptio final Map attributes = new HashMap<>(4); final SchemaIdentifier id = schema.getIdentifier(); -final long schemaId = id.getIdentifier().getAsLong(); -final int schemaVersion = id.getVersion().getAsInt(); +final Long schemaId = id.getIdentifier().getAsLong(); +final Integer schemaVersion = id.getVersion().getAsInt(); attributes.put(HortonworksAttributeSchemaReferenceStrategy.SCHEMA_ID_ATTRIBUTE, String.valueOf(schemaId)); attributes.put(HortonworksAttributeSchemaReferenceStrategy.SCHEMA_VERSION_ATTRIBUTE, String.valueOf(schemaVersion)); --- End diff -- We can do that... one unfortunate thing though is that we will only have the branch name if we also did the lookup with the branch name. If you do the lookup by id+version then nothing I can see on the client API tells you the branch name (i.e. SchemaVersionInfo does not have getBranchName()) ---
[GitHub] nifi pull request #2523: NIFI-4935 Support Schema Branches when using HWX Sc...
Github user bbende commented on a diff in the pull request: https://github.com/apache/nifi/pull/2523#discussion_r174895242 --- Diff: nifi-nar-bundles/nifi-extension-utils/nifi-record-utils/nifi-standard-record-utils/src/main/java/org/apache/nifi/schema/access/HortonworksAttributeSchemaReferenceStrategy.java --- @@ -84,7 +85,8 @@ public RecordSchema getSchema(Map variables, final InputStream c final long schemaId = Long.parseLong(schemaIdentifier); final int version = Integer.parseInt(schemaVersion); --- End diff -- I think I decided to leave the attribute strategy alone to avoid creating extra complexity.. Currently the attributes approach provides a direct lookup by id+version (which would disambiguate the branch) and users are accustomed to using this strategy when they want a specific version. If we then allow the option to use branch instead of version, the combo of id+branch would then get the latest version on the given branch, instead of a direct look up. Also, since we now allow specifying a version with access-by-name strategy, I wonder how often the attributes approach would even be used. If you feel strongly about it we can definitely make it work though. ---
[GitHub] nifi pull request #2523: NIFI-4935 Support Schema Branches when using HWX Sc...
Github user bbende commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2523#discussion_r174888759
--- Diff:
nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/ConfluentSchemaRegistry.java
---
@@ -176,28 +178,33 @@ public void onEnabled(final ConfigurationContext
context) {
return baseUrls;
}
-@Override
-public String retrieveSchemaText(final String schemaName) throws
IOException, SchemaNotFoundException {
-final RecordSchema schema = retrieveSchema(schemaName);
-return schema.getSchemaText().get();
-}
+private RecordSchema retrieveSchemaByName(final SchemaIdentifier
schemaIdentifier) throws IOException, SchemaNotFoundException {
+final Optional schemaName = schemaIdentifier.getName();
+if (!schemaName.isPresent()) {
+throw new
org.apache.nifi.schema.access.SchemaNotFoundException("Cannot retrieve schema
because Schema Name is not present");
+}
-@Override
-public String retrieveSchemaText(final long schemaId, final int
version) throws IOException, SchemaNotFoundException {
-final RecordSchema schema = retrieveSchema(schemaId, version);
-return schema.getSchemaText().get();
+final RecordSchema schema = client.getSchema(schemaName.get());
+return schema;
}
-@Override
-public RecordSchema retrieveSchema(final String schemaName) throws
IOException, SchemaNotFoundException {
-final RecordSchema schema = client.getSchema(schemaName);
+private RecordSchema retrieveSchemaByIdAndVersion(final
SchemaIdentifier schemaIdentifier) throws IOException, SchemaNotFoundException {
--- End diff --
Good point, I'll change that
---
[GitHub] nifi pull request #2523: NIFI-4935 Support Schema Branches when using HWX Sc...
Github user bbende commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2523#discussion_r174888711
--- Diff:
nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/StandardSchemaIdentifier.java
---
@@ -64,6 +75,49 @@ public boolean equals(final Object obj) {
return false;
}
final SchemaIdentifier other = (SchemaIdentifier) obj;
-return getName().equals(other.getName()) &&
getIdentifier().equals(other.getIdentifier()) &&
getVersion().equals(other.getVersion());
+return getName().equals(other.getName())
+&& getIdentifier().equals(other.getIdentifier())
+&& getVersion().equals(other.getVersion())
+&& getBranch().equals(other.getBranch());
+}
+
+/**
+ * Builder to create instances of Schema
--- End diff --
Will update
---
[GitHub] nifi pull request #2523: NIFI-4935 Support Schema Branches when using HWX Sc...
Github user markap14 commented on a diff in the pull request: https://github.com/apache/nifi/pull/2523#discussion_r174826270 --- Diff: nifi-nar-bundles/nifi-extension-utils/nifi-record-utils/nifi-standard-record-utils/src/main/java/org/apache/nifi/schema/access/HortonworksAttributeSchemaReferenceWriter.java --- @@ -40,8 +40,8 @@ public void writeHeader(RecordSchema schema, OutputStream out) throws IOExceptio final Map attributes = new HashMap<>(4); final SchemaIdentifier id = schema.getIdentifier(); -final long schemaId = id.getIdentifier().getAsLong(); -final int schemaVersion = id.getVersion().getAsInt(); +final Long schemaId = id.getIdentifier().getAsLong(); +final Integer schemaVersion = id.getVersion().getAsInt(); attributes.put(HortonworksAttributeSchemaReferenceStrategy.SCHEMA_ID_ATTRIBUTE, String.valueOf(schemaId)); attributes.put(HortonworksAttributeSchemaReferenceStrategy.SCHEMA_VERSION_ATTRIBUTE, String.valueOf(schemaVersion)); --- End diff -- Should we also be adding an attribute now for the Branch Name, if specified? While I realize that it's not explicitly necessary, since the version would disambiguate anything, it may make it more clear when looking at FlowFiles in a queue or looking at provenance data if we have that info. ---
[GitHub] nifi pull request #2523: NIFI-4935 Support Schema Branches when using HWX Sc...
Github user markap14 commented on a diff in the pull request: https://github.com/apache/nifi/pull/2523#discussion_r174825845 --- Diff: nifi-nar-bundles/nifi-extension-utils/nifi-record-utils/nifi-standard-record-utils/src/main/java/org/apache/nifi/schema/access/HortonworksAttributeSchemaReferenceStrategy.java --- @@ -84,7 +85,8 @@ public RecordSchema getSchema(Map variables, final InputStream c final long schemaId = Long.parseLong(schemaIdentifier); final int version = Integer.parseInt(schemaVersion); --- End diff -- Shouldn't we be supporting the use of a Branch Name instead of a Version here? I.e., should we support using either version OR branch name? ---
[GitHub] nifi pull request #2523: NIFI-4935 Support Schema Branches when using HWX Sc...
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2523#discussion_r174821838
--- Diff:
nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/ConfluentSchemaRegistry.java
---
@@ -176,28 +178,33 @@ public void onEnabled(final ConfigurationContext
context) {
return baseUrls;
}
-@Override
-public String retrieveSchemaText(final String schemaName) throws
IOException, SchemaNotFoundException {
-final RecordSchema schema = retrieveSchema(schemaName);
-return schema.getSchemaText().get();
-}
+private RecordSchema retrieveSchemaByName(final SchemaIdentifier
schemaIdentifier) throws IOException, SchemaNotFoundException {
+final Optional schemaName = schemaIdentifier.getName();
+if (!schemaName.isPresent()) {
+throw new
org.apache.nifi.schema.access.SchemaNotFoundException("Cannot retrieve schema
because Schema Name is not present");
+}
-@Override
-public String retrieveSchemaText(final long schemaId, final int
version) throws IOException, SchemaNotFoundException {
-final RecordSchema schema = retrieveSchema(schemaId, version);
-return schema.getSchemaText().get();
+final RecordSchema schema = client.getSchema(schemaName.get());
+return schema;
}
-@Override
-public RecordSchema retrieveSchema(final String schemaName) throws
IOException, SchemaNotFoundException {
-final RecordSchema schema = client.getSchema(schemaName);
+private RecordSchema retrieveSchemaByIdAndVersion(final
SchemaIdentifier schemaIdentifier) throws IOException, SchemaNotFoundException {
--- End diff --
This doesn't appear to take into account a version at all - which I think
is correct because the Confluent Schema Registry I don't supports ID + Version,
as each version would get a different ID? But in this case perhaps the name of
the method should just be `retrieveSchemaById` and not
`retrieveSchemaByIdAndVersion`?
---
[GitHub] nifi pull request #2523: NIFI-4935 Support Schema Branches when using HWX Sc...
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2523#discussion_r174820659
--- Diff:
nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/StandardSchemaIdentifier.java
---
@@ -64,6 +75,49 @@ public boolean equals(final Object obj) {
return false;
}
final SchemaIdentifier other = (SchemaIdentifier) obj;
-return getName().equals(other.getName()) &&
getIdentifier().equals(other.getIdentifier()) &&
getVersion().equals(other.getVersion());
+return getName().equals(other.getName())
+&& getIdentifier().equals(other.getIdentifier())
+&& getVersion().equals(other.getVersion())
+&& getBranch().equals(other.getBranch());
+}
+
+/**
+ * Builder to create instances of Schema
--- End diff --
Think that's a typo - should be "Builder to create instances of Schema
Identifier" - it doesn't build a Schema, just the identifier.
---
[GitHub] nifi pull request #2523: NIFI-4935 Support Schema Branches when using HWX Sc...
GitHub user bbende opened a pull request: https://github.com/apache/nifi/pull/2523 NIFI-4935 Support Schema Branches when using HWX Schema Registry This PR adds optional "Schema Branch" and "Schema Version" properties to the readers/writers which can be used when using the "Schema Name" access strategy. You can specify schema name + branch, or schema name + version, and this will be passed down to the schema registry implementations. Currently only the Hortonworks Schema Registry makes use of the branch concept. This PR also includes some refactoring to clean up the SchemaRegistry interface and move towards a single method to retrieve a schema that takes a schema identifier. Also added unit tests for all of the access strategies and access writers. You can merge this pull request into a Git repository by running: $ git pull https://github.com/bbende/nifi schema-reg-branch Alternatively you can review and apply these changes as the patch at: https://github.com/apache/nifi/pull/2523.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #2523 commit 26b01b6a384cafa50ae262dc9f50e05f41bdb818 Author: Bryan Bende Date: 2018-03-05T22:02:20Z NIFI-4935 Refactoring to support specifying schema branch or schema version when using schema by name strategy ---
