Fixed compilation issues #5
Conversation
Address modernizer violations - use java.util.Objects.requireNonNull - use java.nio.charset.StandardCharsets.US_ASCII Use jetcd-core Update to use refactored ByteSequence and KeyValue classes Update dependency versions NOTE: with this change patch, compilation is successful but karaf is still not starting
| @@ -5,7 +5,7 @@ | |||
| <parent> | |||
| <groupId>org.opendaylight.mdsal</groupId> | |||
| <artifactId>binding-parent</artifactId> | |||
| <version>0.13.1-SNAPSHOT</version> | |||
| <version>0.13.2-SNAPSHOT</version> | |||
There was a problem hiding this comment.
as above, let's use non-SNAPSHOT?
| @@ -130,7 +135,7 @@ | |||
| <dependency> | |||
| <groupId>org.opendaylight.mdsal</groupId> | |||
| <artifactId>mdsal-artifacts</artifactId> | |||
| <version>2.6.0-SNAPSHOT</version> | |||
| <version>3.0.7-SNAPSHOT</version> | |||
There was a problem hiding this comment.
see #4, we could most probably actually use a fixed non-SNAPSHOT mdsal version now, just the same as on current opendaylight master?
There was a problem hiding this comment.
it's better to use 3.0.6, because that matches the version used by controller
| @@ -119,6 +119,11 @@ | |||
| <!-- We MUST override odlparent's too old protobuf version here for jetcd --> | |||
| <version>3.4.0</version> | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>org.ops4j.pax.cdi</groupId> | |||
| <artifactId>pax-cdi-api</artifactId> | |||
There was a problem hiding this comment.
not sure why this is suddenly needed, but it can't really hurt either, so fine with me
There was a problem hiding this comment.
it's because of https://git.opendaylight.org/gerrit/#/c/75762/ (of all people, I should remember!)
| @@ -76,36 +71,23 @@ public void close() { | |||
| } | |||
|
|
|||
| private Watcher watch(long revision) { | |||
| Watcher watcher = etcdWatch.watch(prefix, | |||
There was a problem hiding this comment.
may I ask for some more comments re. why you are removing all this?
There was a problem hiding this comment.
or perhaps you could revert this part of the change, just so that we get the build to work again, and then raise this a separate PR later?
There was a problem hiding this comment.
I now see why you (had to) do this.. this is an impact of etcd-io/jetcd#419 ..
| <groupId>io.etcd</groupId> | ||
| <artifactId>jetcd-core</artifactId> | ||
| --> | ||
| <groupId>io.etcd</groupId> | ||
| <artifactId>jetcd-all</artifactId> |
There was a problem hiding this comment.
are you 100% sure that this is right? I have a (vague) memory that I needed this so that it works in karaf (may be etcd-io/jetcd#434 or etcd-io/jetcd#382, can't remember), and given that you wrote that you're having trouble getting karaf to start, I'd like to have a closer look before merging this
| import static com.google.common.truth.Truth.assertThat; | ||
| import static java.nio.charset.StandardCharsets.UTF_8; | ||
|
|
There was a problem hiding this comment.
This change seems to cause 2 new Checkstyle problems, would you like to fix that up?
[ERROR] src/test/java/org/opendaylight/etcd/launcher/test/EtcdLauncherTest.java:[14] (imports) CustomImportOrder: 'ch.vorburger.exec.ManagedProcessException' should be separated from previous import group by one line.
[ERROR] src/test/java/org/opendaylight/etcd/launcher/test/EtcdLauncherTest.java:[17] (imports) CustomImportOrder: Wrong lexicographical order for 'io.etcd.jetcd.ByteSequence' import. Should be before 'io.etcd.jetcd.KV'.
There was a problem hiding this comment.
There are actually a lot more problems like this in many files ... I've fixed them all up in bd0701a (BTW FYI with https://github.com/vorburger/opendaylight-eclipse-setup you can get Eclipse automatically configured to do the right thing when you press Ctrl-Shift-O to Format Imports; I think something similar is possible for IntelliJ as well). With that, I get test failures in odl-mdsal-broker which we should fix...
|
@bertrandlow I am more or less talking to myself 😈 now... I'm hoping to follow-up with a new PR, inspired by yours, which I'd love you to review. |
|
@bertrandlow see ==> #10 Hope OK for you if I close this? I've not actually used this, but manually done it again from scratch; just because it (my code, not your PR) needed a number of mises à jour and I found it easier to redo them step by step to understand what was causing what error, and how to fix it. This PR has definitely motivated me to finally clean things in this project! Thanks for contributing - will I see more from you? 😃 |
|
Thanks for addressing the build, Michael, and closing my PR. |
Address modernizer violations
Use jetcd-core
Update to use refactored ByteSequence and KeyValue classes
Update dependency versions
NOTE: with this change patch, compilation is successful but karaf is
still not starting