Mck/18049/trunk #2226
Mck/18049/trunk #2226
Conversation
…le-bytes, chronicle-wire, chronicle-threads) Transitive dependencies to these have also been added to raise their versions. asm-* transitive dependencies have been excluded. patch by Mick Semb Wever; reviewed by XXX for CASSANDRA-18049
ekaterinadimitrova2
left a comment
ekaterinadimitrova2
left a comment
There was a problem hiding this comment.
Left some small question and comments
.build/parent-pom-template.xml
Outdated
| <dependency> | ||
| <groupId>net.openhft</groupId> | ||
| <artifactId>chronicle-queue</artifactId> | ||
| <version>5.20.123</version> |
There was a problem hiding this comment.
Which bom are those? It would be nice to add it here in a comment
There was a problem hiding this comment.
no bom, as requested we are upgrading to the latest versions (that are available in the maven1 repo)
There was a problem hiding this comment.
And those latest versions still correspond to specific BOM number, probably latest and I see in the past we were adding those looking at a BOM as my understanding is that the versions of the different dependencies which are listed in a BOM have been integrated and tested to work together.
I was looking into this one - https://mvnrepository.com/artifact/net.openhft/chronicle-bom/2.23.137
Then I also realized that we do not use the latest ea release (as we see we do with posix) but the previous one
https://mvnrepository.com/artifact/net.openhft/chronicle-queue
At first I thought you do not want to use ea version, but then you used for posix?
I think adding the BOM number in a comment reminds people not to update separate dependency when it is known that some should be upgraded together to tested compatible versions
There was a problem hiding this comment.
I started off with the bom, then going through maven1 repo used the versions available (basically aligning to the bom, some patch versions differed).
Then fixing tests some versions needed bumping, posix the example here.
Then on the dev@ thread folk requested we use the latest versions available. The versions that then got bumped made sense to as they matched sibling libraries we already had (e.g. jna*), and/or were about newer patch versions supporting more arch/platforms.
While the initial goal may have been to re-use the tested QA of the bom's declaration, that's evolved to tailoring it best (with evidence) to our needs and relying on being in trunk to do our own testing and QA. (IMO this approach would not be appropriate in one of our release branches.)
There was a problem hiding this comment.
I am still not sure why posix is at ea latest version but the chronicle-* dependencies are at non-ea latest versions.
Also, maybe a typo but chronicle-core has 2.23.37, you have 36 here.
There was a problem hiding this comment.
I am still not sure why posix is at ea latest version but the chronicle-* dependencies are at non-ea latest versions.
Only the bump of posix was needed to fix the related failures.
( Many, and newer, versions are simply not found in https://repo1.maven.org/maven2/net/openhft/ )
Also, maybe a typo but chronicle-core has 2.23.37, you have 36 here.
While chronicle-core-2.23.37 itself is available publicly, it's parent pom is not, and uses other dependencies that are SNAPSHOTs. So that version is out for us.
There was a problem hiding this comment.
Thank you for explaining, I see what you mean now!
Can we add a few sentences to the ticket about that? I know people who are rebasing their forks are doing another pass of review so it might be helpful for context as it is not immediately obvious.
|
pushed update: michaelsembwever@60732e4 |
ekaterinadimitrova2
left a comment
ekaterinadimitrova2
left a comment
There was a problem hiding this comment.
This seems ready for another CI round. +1 on clean CI
what's changed that's need another CI run? |
belliottsmith
force-pushed
the
trunk
branch
2 times, most recently
from
df3eb40 to
54e39a9
Compare
2 participants
No description provided.