Feature/abris 246 support for multiple spark avro #247
Conversation
src/main/scala/za/co/absa/abris/avro/registry/ConfluentMockRegistryClient.scala
Outdated
Show resolved
Hide resolved
| val settings = new KafkaAvroDeserializerConfig(configs.asJava) | ||
| val urls = settings.getSchemaRegistryUrls | ||
| val maxSchemaObject = settings.getMaxSchemasPerSubject | ||
|
|
There was a problem hiding this comment.
Where did the logic for the mock urls go?
There was a problem hiding this comment.
We already have AbrisConfig.REGISTRY_CLIENT_CLASS that allows you to theoretically set any registry, even something completely different from confluent. I don't think we need another way of setting mock registry.
Another issue is that KafkaAvroDeserializerConfig is used to get this mock setting which is tied to external code. I would like this dependency to be encapsulated inside the registry client abstraction only.
There was a problem hiding this comment.
Hmm. I think we should keep that logic somewhere, because exactly this functionality was recently added #180. If I understand correctly, the dependency on KafkaAvroDeserializerConfig is only used to get the schema registry urls. I guess that can also be solved differently, e.g. using za.co.absa.abris.config.SCHEMA_REGISTRY_URL
There was a problem hiding this comment.
Ok for backward compatibility it makes sense. Maybe ConfluentRegistryClient could have factory method that would check this. That way, it would be encapsulated there as well.
There was a problem hiding this comment.
At the end I did what you suggested, seems to be cleaner approach after all
There was a problem hiding this comment.
Compared to the original logic, throwing error messages in case of multiple schema registry urls is not covered. The behaviour would be different if someone passed a mock url and a real url. Previously that would have thrown an exception, now it will proceed to create the mock registry. I think that's acceptable. It shouldn't break anything.
src/main/scala/za/co/absa/abris/avro/read/confluent/SchemaManagerFactory.scala
Outdated
Show resolved
Hide resolved
cerveada
force-pushed
the
feature/abris-246-support-for-multiple-spark-avro
branch
from
44a56dd to
3a9b32d
Compare
- add support for spark-avro 2.4 and newer - add support for schema registry 5.3 and newer
cerveada
force-pushed
the
feature/abris-246-support-for-multiple-spark-avro
branch
from
3a9b32d to
1a12e38
Compare
| val settings = new KafkaAvroDeserializerConfig(configs.asJava) | ||
| val urls = settings.getSchemaRegistryUrls | ||
| val maxSchemaObject = settings.getMaxSchemasPerSubject | ||
|
|
There was a problem hiding this comment.
Compared to the original logic, throwing error messages in case of multiple schema registry urls is not covered. The behaviour would be different if someone passed a mock url and a real url. Previously that would have thrown an exception, now it will proceed to create the mock registry. I think that's acceptable. It shouldn't break anything.
Abris #246 code review changes II
cerveada
force-pushed
the
feature/abris-246-support-for-multiple-spark-avro
branch
from
1a12e38 to
3ca3319
Compare
cerveada
deleted the
feature/abris-246-support-for-multiple-spark-avro
branch
Since there are already multiple PR's in the pipeline I include the commits in the branch, later this can be rebased