----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/73286/#review223588 -----------------------------------------------------------
addons/kafka-bridge/pom.xml Lines 161 (patched) <https://reviews.apache.org/r/73286/#comment312671> why hive-exec dependency is needed here? I see you use JSON libs from hive-exec. Consider removing hive dependency and use json-simple lib. addons/kafka-bridge/src/main/java/org/apache/atlas/kafka/bridge/KafkaBridge.java Lines 63 (patched) <https://reviews.apache.org/r/73286/#comment312672> KAFKA_SCHEMA_REGISTRY => KAFKA_SCHEMA_REGISTRY_HOSTNAME addons/kafka-bridge/src/main/java/org/apache/atlas/kafka/bridge/KafkaBridge.java Lines 102 (patched) <https://reviews.apache.org/r/73286/#comment312683> "Entering custom Kafka bridge" => "Kafka bridge" addons/kafka-bridge/src/main/java/org/apache/atlas/kafka/bridge/KafkaBridge.java Lines 136 (patched) <https://reviews.apache.org/r/73286/#comment312670> consider creating a static constant for "KAFKA_SCHEMA_REGISTRY". Something like: private static final String KAFKA_SCHEMA_REGISTRY_ENV_VARIABLE = "KAFKA_SCHEMA_REGISTRY"; Also, add null check for System.getenv() addons/kafka-bridge/src/main/java/org/apache/atlas/kafka/bridge/KafkaBridge.java Lines 185 (patched) <https://reviews.apache.org/r/73286/#comment312684> change to LOG.error("..., e"). Consider not printing stacktrace here. addons/kafka-bridge/src/main/java/org/apache/atlas/kafka/bridge/KafkaBridge.java Lines 200 (patched) <https://reviews.apache.org/r/73286/#comment312685> nit: follow vertical alignment of variable declaration. addons/kafka-bridge/src/main/java/org/apache/atlas/kafka/bridge/KafkaBridge.java Lines 371 (patched) <https://reviews.apache.org/r/73286/#comment312687> replace har-coded "atlas.metadata.namespace" with KAFKA_METADATA_NAMESPACE addons/kafka-bridge/src/main/java/org/apache/atlas/kafka/bridge/KafkaBridge.java Lines 382 (patched) <https://reviews.apache.org/r/73286/#comment312688> consider moving the sorting logic: line 382-392 inside createNestedFields() method. addons/kafka-bridge/src/main/java/org/apache/atlas/kafka/bridge/KafkaBridge.java Lines 394 (patched) <https://reviews.apache.org/r/73286/#comment312689> why set nested fields in attributes? Shouldn't this be only on the relationshipAttributes? "attributes" of AtlasEntity should contain only primitive attributes and all relationships (createdFields) should be added only to relationshipAttributes addons/kafka-bridge/src/main/java/org/apache/atlas/kafka/bridge/KafkaBridge.java Lines 407 (patched) <https://reviews.apache.org/r/73286/#comment312690> why hard-code to string? compare with ENUM: if (field.schema().getType() == Schema.Type.ARRAY) addons/kafka-bridge/src/main/java/org/apache/atlas/kafka/bridge/KafkaBridge.java Lines 417 (patched) <https://reviews.apache.org/r/73286/#comment312691> why hard-code to string? compare with ENUM: if (field.schema().getType() == Schema.Type.RECORD) addons/kafka-bridge/src/main/java/org/apache/atlas/kafka/bridge/KafkaBridge.java Lines 418 (patched) <https://reviews.apache.org/r/73286/#comment312692> move this 'if' condition inside else-if above line 417 addons/kafka-bridge/src/main/java/org/apache/atlas/kafka/bridge/KafkaBridge.java Lines 476 (patched) <https://reviews.apache.org/r/73286/#comment312686> 'attributes' is not used other than 476. atlasClientV2.getEntityByAttribute(typeName, Collections.singletonMap(ATTRIBUTE_QUALIFIED_NAME, qualifiedName)); addons/kafka-bridge/src/main/java/org/apache/atlas/kafka/bridge/SchemaRegistryConnector.java Lines 45 (patched) <https://reviews.apache.org/r/73286/#comment312676> replace hardcoded 200 to HttpStatus.SC_OK addons/kafka-bridge/src/main/java/org/apache/atlas/kafka/bridge/SchemaRegistryConnector.java Lines 56 (patched) <https://reviews.apache.org/r/73286/#comment312675> considering printing list content with separator: String.join(", ", list); addons/kafka-bridge/src/main/java/org/apache/atlas/kafka/bridge/SchemaRegistryConnector.java Lines 57 (patched) <https://reviews.apache.org/r/73286/#comment312673> consider defining own logger for 'SchemaRegistryConnector' instead of using KafkaBridge's logger. addons/kafka-bridge/src/main/java/org/apache/atlas/kafka/bridge/SchemaRegistryConnector.java Lines 62 (patched) <https://reviews.apache.org/r/73286/#comment312681> consider catching all Exception as: catch (Exception e) { ..... } addons/kafka-bridge/src/main/java/org/apache/atlas/kafka/bridge/SchemaRegistryConnector.java Lines 65 (patched) <https://reviews.apache.org/r/73286/#comment312674> LOG.error should be added here. Consider print error stack within logger message and avoid line 63. LOG.error("......", e) addons/kafka-bridge/src/main/java/org/apache/atlas/kafka/bridge/SchemaRegistryConnector.java Lines 69 (patched) <https://reviews.apache.org/r/73286/#comment312677> replace 404 with HttpStatus.SC_NOT_FOUND addons/kafka-bridge/src/main/java/org/apache/atlas/kafka/bridge/SchemaRegistryConnector.java Lines 72 (patched) <https://reviews.apache.org/r/73286/#comment312678> "Can not find versions to schema: {} in Kafka" => "No schema versions found for schema: {} in Schema Registry" addons/kafka-bridge/src/main/java/org/apache/atlas/kafka/bridge/SchemaRegistryConnector.java Lines 102 (patched) <https://reviews.apache.org/r/73286/#comment312679> create static constant for "schema" addons/kafka-bridge/src/main/java/org/apache/atlas/kafka/bridge/SchemaRegistryConnector.java Lines 104 (patched) <https://reviews.apache.org/r/73286/#comment312680> avoid printing stacktrace here and print in log: LOG.error ("....", e) addons/kafka-bridge/src/main/java/org/apache/atlas/kafka/bridge/SchemaRegistryConnector.java Lines 113 (patched) <https://reviews.apache.org/r/73286/#comment312682> "Can not" -> "Cannot" - Sarath Subramanian On Oct. 5, 2021, 1:06 a.m., Aileen Toleikis wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/73286/ > ----------------------------------------------------------- > > (Updated Oct. 5, 2021, 1:06 a.m.) > > > Review request for atlas. > > > Repository: atlas > > > Description > ------- > > The Kafka Community is using Schema Registry more and more heavily but as > Atlas is currently unaware of this, this extension helps Atlas make use of > the Schemas. > > We have tested this extension and we have production environments where Atlas > will not be allowed without schema registry access. We have received feedback > that this extension would be sufficient to allow production use. > > Changes: > > initial commit > > > added schema creation in Atlas > > > added Schema creation, Schema linkage and started to work on versions > > > avro_field creation is working. However, there is no connection to schemas yet > > > smaller refactorings and improved logging > > > added field reference in schemas > > > deleted second README and added gitignore > > > Smaller changes after rebasing on new version > > > moved HTTP REST calls to a separate class (SchemaRegistryConnector.java) > > > added testCreateSchema() as test case > > > added testCreateField() and testUpdateField() as test case > > > added testGetSchemas() and testGetSchemaVersions() as test case > > > added testUpdateSchema() as test case > > > removed unnecessary comments > > > Diffs > ----- > > addons/kafka-bridge/pom.xml 2ac19fd20e0c0322b0d104641ddd5a2ef89bf9d0 > > addons/kafka-bridge/src/main/java/org/apache/atlas/kafka/bridge/KafkaBridge.java > f9548244364a8f79f346411739348d0d53298c99 > > addons/kafka-bridge/src/main/java/org/apache/atlas/kafka/bridge/SchemaRegistryConnector.java > PRE-CREATION > > addons/kafka-bridge/src/main/java/org/apache/atlas/kafka/model/KafkaDataTypes.java > 0f81b4c37a1d8f84c587a914eb460b290ae36987 > > addons/kafka-bridge/src/test/java/org/apache/atlas/kafka/bridge/KafkaBridgeTest.java > f86ceb58fd250d80f3ae3c71085c8c0adbc9116b > pom.xml 47768e9fa7d8abcfc81cd148917400ac478a333a > > > Diff: https://reviews.apache.org/r/73286/diff/3/ > > > Testing > ------- > > > Thanks, > > Aileen Toleikis > >
