agile-lab-dev / darwin

Avro Schema Evolution made easy
Apache License 2.0
34 stars 10 forks source link

Confluent rest connector #86

Closed andrea-rockt closed 3 years ago

andrea-rockt commented 3 years ago

TBD

andrea-rockt commented 3 years ago

@tmnd1991

How can I cross build for 2.13 without this warning (that becomes an error)?

[error] /Users/andreafonti/repositories/github.com/andrea-rockt/darwin/confluent/src/main/scala/it/agilelab/darwin/connector/confluent/ConfluentConnector.scala:36:12: object JavaConverters in package collection is deprecated (since 2.13.0): Use `scala.jdk.CollectionConverters` instead
[error]     client.getAllSubjects.asScala.toList.flatMap { subject =>
[error]            ^

do we already have a facility for this?

silencer plugin maybe?

tmnd1991 commented 3 years ago

@tmnd1991

How can I cross build for 2.13 without this warning (that becomes an error)?

[error] /Users/andreafonti/repositories/github.com/andrea-rockt/darwin/confluent/src/main/scala/it/agilelab/darwin/connector/confluent/ConfluentConnector.scala:36:12: object JavaConverters in package collection is deprecated (since 2.13.0): Use `scala.jdk.CollectionConverters` instead
[error]     client.getAllSubjects.asScala.toList.flatMap { subject =>
[error]            ^

do we already have a facility for this?

silencer plugin maybe?

it.agilelab.darwin.common.compat is your friend

tmnd1991 commented 3 years ago

As a side note:

I've just realised that def toJava[A](iterable: scala.collection.Iterable[A]): java.lang.Iterable[A] is a "bit" broken because eagerly collects the Iterable, that might not be already done, if you feel like it, you could change to something like this:

def toJava[A](iterable: scala.collection.Iterable[A]): java.lang.Iterable[A] = new lang.Iterable[A] {
    override def iterator(): util.Iterator[A] = new util.Iterator[A] {
      private val it                = iterable.iterator
      override def hasNext: Boolean = it.hasNext
      override def next(): A = it.next()
    }
  }
tmnd1991 commented 3 years ago

This review will be so "meta" I will cry 😭 😂

I see the work you've done, and I feel it's legit, I also think it needs a "major" change of API to have decent guarantees that it won't break the whole world 😄

With these changes I would also move the AvroSingleObjectEncoding API to developer/internal API and make the whole header + id more "configurable" so that we could easily change support different headers with different IDs and different fingerprinting in the future without major API breakage.

andrea-rockt commented 3 years ago

@tmnd1991 I applied the proposed fixes and updated the README, if you find this feature ready we can merge!

Thanks a lot