Skip to content

Conversation

@nastra
Copy link
Contributor

@nastra nastra commented Oct 29, 2021

Uses currently 0.13.0-SNAPSHOT until 0.12.1 is actually released

@codecov
Copy link

codecov bot commented Oct 29, 2021

Codecov Report

Merging #2543 (66269e5) into main (e84a8ea) will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #2543      +/-   ##
============================================
+ Coverage     84.62%   84.64%   +0.02%     
- Complexity     1942     1943       +1     
============================================
  Files           270      270              
  Lines         11145    11145              
  Branches        802      802              
============================================
+ Hits           9431     9434       +3     
+ Misses         1399     1396       -3     
  Partials        315      315              
Flag Coverage Δ
java 84.47% <ø> (+0.03%) ⬆️
javascript 85.58% <ø> (ø)
python 85.89% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...nessie/versioned/persist/tx/TxDatabaseAdapter.java 74.94% <0.00%> (+0.63%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e84a8ea...66269e5. Read the comment docs.

Copy link
Member

@snazy snazy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not merge SNAPSHOT dependencies.

@nastra
Copy link
Contributor Author

nastra commented Oct 29, 2021

Let's not merge SNAPSHOT dependencies.

Yep that was definitely not my intention. I rather wanted to test that the newly published Iceberg snapshots can be consumed and that the current code in Nessie works with the latest Iceberg version that's coming from master.

Once 0.12.1 is out, the snapshot repo will be removed again in this change here.

@snazy
Copy link
Member

snazy commented Oct 29, 2021

Let's not merge SNAPSHOT dependencies.

Yep that was definitely not my intention. I rather wanted to test that

Mind marking this PR as a draft then?

@nastra nastra marked this pull request as draft October 29, 2021 09:04
@nastra nastra marked this pull request as ready for review November 9, 2021 10:23
<guava.version>31.0.1-jre</guava.version>
<hadoop.version>3.3.1</hadoop.version>
<iceberg.version>0.12.0</iceberg.version>
<iceberg.version>0.12.1</iceberg.version>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to upgrade iceberg version when it cannot work with current Nessie ?
Doesn't it make sense to upgrade once iceberg side PR is merged ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have a circular dependency in the project with Nessie+Iceberg. The SQL extensions depend on iceberg.version, which uses a Nessie version older than the current one, so this is independent from the PR you mentioned

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. got it.

Now for this iceberg.version to work we are keeping nessie-client @ 0.9.2 in our pom even though we are in 0.12.1 version.

@rymurr , @snazy , @nastra : Should we move this ITNessieStatements to Nessie module in iceberg code ? so that Nessie code will be clean and always points to latest version for client as-well ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ajantha-bhat client.nessie.version points to 0.9.2 because that's the version that is being used by iceberg 0.12.1 currently. Also we can't move ITNessieStatements to Iceberg because the SQL extensions are part of the Nessie codebase, so the test should also be in the Nessie codebase

@nastra nastra requested a review from snazy November 9, 2021 11:27
@nastra nastra merged commit 4d55f2a into projectnessie:main Nov 9, 2021
@nastra nastra deleted the bump-iceberg branch November 9, 2021 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants