Moved the ZooKeeper 3.5.x compatibility check to targets other than main #3203
Conversation
|
I think this might be syntactically correct as it did not run the ZooKeeper compatibility check for this PR. Would need to check that a PR targeting 2.1 or 1.10 does run the check after this PR is merged. Waiting to merge this until I get confirmation that this is ok to do for 3.0. |
There was a problem hiding this comment.
I'm not sure what's wrong with the current mechanism. I don't know why we would exclude the main branch. If the intent is to no longer support Hadoop 3.0.x and ZK 3.5.x for Accumulo 3.0 and later, that's fine, but a much simpler solution is to simply delete that line from the matrix, so we don't do any more compatibility testing for these for Accumulo 3.0 and later (other branches wouldn't be affected, because the change would only occur to the workflow definitions in the main branch). That would effectively make whatever we have in our POM the minimum supported versions for 3.0 and later (because that's all we'd be testing).
However, I don't see a compelling reason to do this. People upgrade Hadoop and ZooKeeper independently of Accumulo, as these are the only two dependencies that we don't actually bundle with our binary distribution tarball. This workflow matrix line merely checks that some earlier versions users may be using aren't obviously broken.
I think it's fine if we leave Hadoop 3.0 and ZK 3.5 the minimum, and continue checking it for the main branch. Unless there's some reason we want to drop them... perhaps because of some newer feature we want to leverage that isn't available in these older versions? If so, a known incompatibility like that would be a good reason to bump the minimum that we're testing with.
| env: | ||
| MAVEN_OPTS: -Djansi.force=true | ||
| - name: Build with Maven (ZooKeeper compatibility build) | ||
| if: ${{ github.base_ref != 'main' }} |
There was a problem hiding this comment.
Okay, that's a compelling reason to bump the minimum. What's the minimum for it to work? Can we just modify the one existing line, rather than do the changes proposed in this PR that jumps through hoops to check for main?
There was a problem hiding this comment.
According to this, it looks like 3.6.0 might be the minimum.
There was a problem hiding this comment.
I will modify the GitHub action file in my branch and see if that resolves it
There was a problem hiding this comment.
If that's the case, then changing it to 3.6.4 (the latest 3.6 bugfix) would probably be appropriate.
There was a problem hiding this comment.
3.6.1 was the minimum. I updated it in my PR, I'm going to close this.
There was a problem hiding this comment.
3.6.1 is the minimum from an API perspective. Do you think the API changed between 3.6.1 and 3.6.4 that would cause an issue, but still works with 3.8.1?
There was a problem hiding this comment.
I hope not. That would be uncharacteristically unstable of the ZK team.
| run: mvn -B -V -e -ntp "-Dstyle.color=always" -DskipFormat package -DskipTests -Dhadoop.version=3.0.3 -Dzookeeper.version=3.5.10 | ||
| env: |
There was a problem hiding this comment.
This just puts inline what we have previously defined elsewhere. It seems the reason to do this is to exclude the main branch, and not use the normal args, but I'm not sure why we're excluding the main branch.
|
Superseded by patch to update to 3.6 in #3200 |
2 participants
Closes #3201