Add hashOnRef query param to support time travel on a named ref by nastra · Pull Request #1589 · projectnessie/nessie

nastra · 2021-07-13T16:27:25Z

This change is

snazy

I just have one comment (but I was so mean to annotate every occurrence ;) ) - it's about the commit-log: since we effectively add the "start hash", it should be named "start hash" and since we're already there, it wouldn't be much of an effort to add the "end hash" parameter as well (maybe need to implement a Spliterator for the "end hash" though).

snazy · 2021-07-13T18:38:24Z

clients/client/src/main/java/org/projectnessie/client/ClientTreeApi.java

  @Override
  public LogResponse getCommitLog(
-      String ref, Integer maxRecords, String pageToken, String queryExpression)
+      String ref, String hashOnRef, Integer maxRecords, String pageToken, String queryExpression)


Guess this should be startHash instead of hashOnRef. Since we're already changing this, can you add endHash as welll?:

I would rather prefer to have this in a separate commit/PR since it's a slightly different feature

snazy · 2021-07-13T18:41:05Z

clients/client/src/main/java/org/projectnessie/client/StreamingUtil.java

  public static Stream<CommitMeta> getCommitLogStream(
      @NotNull TreeApi treeApi,
      @NotNull String ref,
+      String hashOnRef,


(similar to above about startHash instead of hashOnRef + endHash)

snazy · 2021-07-13T18:42:01Z

model/src/main/java/org/projectnessie/api/TreeApi.java

              examples = {@ExampleObject(ref = "ref")})
          @PathParam("ref")
          String ref,
+      @Nullable


(similar to above about startHash instead of hashOnRef + endHash)

snazy · 2021-07-13T18:43:12Z

servers/services/src/main/java/org/projectnessie/services/rest/TreeResource.java

  public LogResponse getCommitLog(
-      String ref, Integer maxRecords, String pageToken, String queryExpression)
+      String namedRef,
+      String hashOnRef,


(similar to above about startHash instead of hashOnRef + endHash)

snazy · 2021-07-13T18:43:27Z

ui/src/utils/api/apis/DefaultApi.ts


 export interface GetCommitLogRequest {
    ref: string;
+    hashOnRef?: string;


(similar to above about startHash instead of hashOnRef + endHash)

codecov · 2021-07-14T06:48:54Z

Codecov Report

Merging #1589 (239c8a8) into main (6b2c1c8) will increase coverage by 0.12%.
The diff coverage is 89.41%.

@@             Coverage Diff              @@
##               main    #1589      +/-   ##
============================================
+ Coverage     77.24%   77.37%   +0.12%     
- Complexity     2199     2216      +17     
============================================
  Files           289      289              
  Lines         13576    13690     +114     
  Branches       1035     1037       +2     
============================================
+ Hits          10487    10592     +105     
- Misses         2605     2616      +11     
+ Partials        484      482       -2

Flag	Coverage Δ
java	`76.65% <89.30%> (+0.14%)`	⬆️
python	`86.61% <100.00%> (+0.03%)`	⬆️

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

Impacted Files	Coverage Δ
...ark/sql/execution/datasources/v2/NessieUtils.scala	`0.00% <0.00%> (ø)`
...ark/sql/execution/datasources/v2/ShowLogExec.scala	`0.00% <0.00%> (ø)`
...n/java/org/projectnessie/client/StreamingUtil.java	`50.00% <50.00%> (ø)`
.../org/projectnessie/services/rest/TreeResource.java	`70.28% <50.00%> (ø)`
...n/java/org/projectnessie/hms/TransactionStore.java	`58.62% <66.66%> (ø)`
.../projectnessie/services/rest/ContentsResource.java	`75.00% <66.66%> (ø)`
.../org/projectnessie/services/rest/BaseResource.java	`80.48% <86.95%> (+12.30%)`	⬆️
...java/org/projectnessie/jaxrs/AbstractTestRest.java	`95.55% <93.10%> (-0.25%)`	⬇️
...va/org/projectnessie/client/ClientContentsApi.java	`100.00% <100.00%> (ø)`
...n/java/org/projectnessie/client/ClientTreeApi.java	`96.34% <100.00%> (+0.09%)`	⬆️
... and 5 more

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 6b2c1c8...239c8a8. Read the comment docs.

rymurr · 2021-07-14T10:25:29Z

servers/services/src/main/java/org/projectnessie/services/rest/BaseResource.java

+  WithHash<NamedRef> namedRefWithHashOrThrow(String namedRef, @Nullable String hashOnRef)
+      throws NessieNotFoundException {
+    List<WithHash<NamedRef>> collect =
+        store


how often is this called? This could be expensive and I don't quite understand the purpose

it's called in 4 places (getCommitLog / getEntries / getContents / getMultipleContents) and we will eventually have a faster alternative in in the VersionStoreV2

Apply changes to Iceberg required by API changes in Nessie: * [Re-introduce wrapper classes for query params of CommitLog/Entries](projectnessie/nessie#1595) * [Server-side commit range filtering](projectnessie/nessie#1596) * [Add hashOnRef query param to support time travel on a named ref](projectnessie/nessie#1589) * [Only accept NamedRefs in REST API](projectnessie/nessie#1583)

* Bump Nessie to 0.8.2 + replace Gradle plugin with new JUnit extension More changes in this PR in following commits. Replace Gradle plugin with new JUnit extension. See [Add JAX-RS tests and add JUnit/Jupyter extension](projectnessie/nessie#1566) * Changes required by Nessie-API changes Apply changes to Iceberg required by API changes in Nessie: * [Re-introduce wrapper classes for query params of CommitLog/Entries](projectnessie/nessie#1595) * [Server-side commit range filtering](projectnessie/nessie#1596) * [Add hashOnRef query param to support time travel on a named ref](projectnessie/nessie#1589) * [Only accept NamedRefs in REST API](projectnessie/nessie#1583) * Bugfix: must send the Contents.id of the existing table Nessie's `Contents.id` is a random ID generated when the `Contents.Key` is first used (think: CREATE TABLE) and must not be changed. This change addresses a bug in the Iceberg-Nesie code that caused a new id for every change. * Throw `CommitStateUnknownException` for `renameTable` as well Follow-up of #2515 * Fix race-condition & save one roundtrip to Nessie during "commit" When commiting a change, the Nessie-API now returns the hash of the commit for the change. This returned hash should then be used as the "expected hash" for the next commit. The previous approach was to commit the change to Nessie and then do another request to retrieve the new hash of HEAD. This old approach is prone to a race condition, namely when another commit happens after "this" commit but before retrieving the "new HEAD", so "this" instance would wrongly ignore the other commit's changes during conflict checks. See [Let VersionStore.create()+commit() return the current hash](projectnessie/nessie#1089)

* Bump Nessie to 0.8.2 + replace Gradle plugin with new JUnit extension More changes in this PR in following commits. Replace Gradle plugin with new JUnit extension. See [Add JAX-RS tests and add JUnit/Jupyter extension](projectnessie/nessie#1566) * Changes required by Nessie-API changes Apply changes to Iceberg required by API changes in Nessie: * [Re-introduce wrapper classes for query params of CommitLog/Entries](projectnessie/nessie#1595) * [Server-side commit range filtering](projectnessie/nessie#1596) * [Add hashOnRef query param to support time travel on a named ref](projectnessie/nessie#1589) * [Only accept NamedRefs in REST API](projectnessie/nessie#1583) * Bugfix: must send the Contents.id of the existing table Nessie's `Contents.id` is a random ID generated when the `Contents.Key` is first used (think: CREATE TABLE) and must not be changed. This change addresses a bug in the Iceberg-Nesie code that caused a new id for every change. * Throw `CommitStateUnknownException` for `renameTable` as well Follow-up of apache#2515 * Fix race-condition & save one roundtrip to Nessie during "commit" When commiting a change, the Nessie-API now returns the hash of the commit for the change. This returned hash should then be used as the "expected hash" for the next commit. The previous approach was to commit the change to Nessie and then do another request to retrieve the new hash of HEAD. This old approach is prone to a race condition, namely when another commit happens after "this" commit but before retrieving the "new HEAD", so "this" instance would wrongly ignore the other commit's changes during conflict checks. See [Let VersionStore.create()+commit() return the current hash](projectnessie/nessie#1089)

Add hashOnRef query param to support time travel on a named ref

519451d

nastra marked this pull request as draft July 13, 2021 16:27

snazy reviewed Jul 13, 2021

View reviewed changes

fix defaulting to branch

239c8a8

nastra marked this pull request as ready for review July 14, 2021 06:56

nastra requested a review from snazy July 14, 2021 09:12

snazy approved these changes Jul 14, 2021

View reviewed changes

rymurr approved these changes Jul 14, 2021

View reviewed changes

rymurr reviewed Jul 14, 2021

View reviewed changes

nastra merged commit 152a867 into projectnessie:main Jul 14, 2021

nastra deleted the hashOnRef-for-time-travel-support branch July 14, 2021 10:34

snazy mentioned this pull request Jul 15, 2021

Bump Nessie to 0.8.2 + related changes apache/iceberg#2588

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add hashOnRef query param to support time travel on a named ref#1589

Add hashOnRef query param to support time travel on a named ref#1589
nastra merged 2 commits intoprojectnessie:mainfrom
nastra:hashOnRef-for-time-travel-support

nastra commented Jul 13, 2021 •

edited by jacques-n

Loading

Uh oh!

snazy left a comment

Uh oh!

snazy Jul 13, 2021

Uh oh!

nastra Jul 14, 2021

Uh oh!

snazy Jul 13, 2021

Uh oh!

snazy Jul 13, 2021

Uh oh!

snazy Jul 13, 2021

Uh oh!

snazy Jul 13, 2021

Uh oh!

codecov bot commented Jul 14, 2021 •

edited

Loading

Uh oh!

rymurr Jul 14, 2021

Uh oh!

nastra Jul 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

nastra commented Jul 13, 2021 • edited by jacques-n Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snazy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nastra commented Jul 13, 2021 •

edited by jacques-n

Loading

codecov bot commented Jul 14, 2021 •

edited

Loading