Skip to content

Conversation

@snazy
Copy link
Member

@snazy snazy commented May 14, 2021

The Delta clients can run into an unrecoverable (dead) state, when the reference changed in the mean time.
This commit adds a single retry in case the client throws a NessieConflictException.

The Delta-Client kept a single Reference that was initialized with the named-reference given
in the hadoop-config. However, if the configuration changed to switch to another Nessie branch,
that Reference was never refreshed and resulted into NessieNotFoundExceptions.

The change is to replace that single Reference field with a map of ref-name to Reference
to support multiple branches.

Also adds tests to verify that the Delta client works with multiple branches and the retry
when hitting a NessieConflictExcepiton.

The PR also changes the three delta-modules to include the resources from the -core module for tests.


This change is Reviewable

@codecov
Copy link

codecov bot commented May 14, 2021

Codecov Report

Merging #1249 (e527d32) into main (d591ae8) will decrease coverage by 0.90%.
The diff coverage is 81.81%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1249      +/-   ##
============================================
- Coverage     72.25%   71.34%   -0.91%     
  Complexity      142      142              
============================================
  Files           232      242      +10     
  Lines          9233     9381     +148     
  Branches        839      855      +16     
============================================
+ Hits           6671     6693      +22     
- Misses         2127     2250     +123     
- Partials        435      438       +3     
Flag Coverage Δ Complexity Δ
java 69.98% <81.81%> (-0.99%) 142.00 <0.00> (ø)
python 83.93% <ø> (ø) 0.00 <ø> (ø)

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

Impacted Files Coverage Δ Complexity Δ
...a/org/projectnessie/deltalake/NessieLogStore.scala 70.39% <81.81%> (+2.12%) 0.00 <0.00> (ø)
...ie/server/providers/DynamoVersionStoreFactory.java 0.00% <0.00%> (ø) 0.00% <0.00%> (?%)
...ssie/server/providers/ResteasyExceptionMapper.java 0.00% <0.00%> (ø) 0.00% <0.00%> (?%)
...ctnessie/server/config/JGitVersionStoreConfig.java 0.00% <0.00%> (ø) 0.00% <0.00%> (?%)
.../server/providers/InMemoryVersionStoreFactory.java 0.00% <0.00%> (ø) 0.00% <0.00%> (?%)
...sie/server/filters/SinglePageAppRoutingFilter.java 0.00% <0.00%> (ø) 0.00% <0.00%> (?%)
.../org/projectnessie/server/providers/StoreType.java 0.00% <0.00%> (ø) 0.00% <0.00%> (?%)
...ver/providers/ConfigurableVersionStoreFactory.java 0.00% <0.00%> (ø) 0.00% <0.00%> (?%)
...ssie/server/providers/JGitVersionStoreFactory.java 0.00% <0.00%> (ø) 0.00% <0.00%> (?%)
...tnessie/server/providers/RegisterObjectMapper.java 0.00% <0.00%> (ø) 0.00% <0.00%> (?%)
... 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 d591ae8...e527d32. Read the comment docs.

@snazy snazy force-pushed the delta-retry-on-conflict branch from 7524732 to 55de2e7 Compare May 17, 2021 11:50
@snazy snazy changed the title Handle NessieConflictException in Delta clients Handle NessieConflictException in Delta clients + handling of multiple-branches May 17, 2021
@snazy snazy force-pushed the delta-retry-on-conflict branch from 55de2e7 to ab1fee6 Compare May 17, 2021 14:09
snazy added 2 commits May 19, 2021 11:49
…ple-branches

The Delta clients can run into an unrecoverable (dead) state, when the reference changed in the mean time.
This commit adds a single retry in case the client throws a `NessieConflictException`.

The Delta-Client kept a single `Reference` that was initialized with the named-reference given
in the hadoop-config. However, if the configuration changed to switch to another Nessie branch,
that `Reference` was never refreshed and resulted into `NessieNotFoundException`s.

The change is to replace that single `Reference` field with a map of ref-name to `Reference`
to support multiple branches.

Also adds tests to verify that the Delta client works with multiple branches and the retry
when hitting a `NessieConflictExcepiton`. The new tests are disabled for Spark2/Delta0.6.
/**
* Keeps a mapping of reference name to current hash.
*/
private val referenceMap: util.Map[String, Reference] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

why a java map and not a scala map?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just need a mutable map here - and using a Java HashMap felt to be the easiest way to go.

// Delta < 0.8 w/ Spark 2.x doesn't support multiple branches well (warnings when changing the configuration)
@DisabledIfSystemProperty(named = "skip-multi-branch-tests", matches = "true")
void testCommitRetry() throws Exception {
String csvSalaries1 = ITDeltaLog.class.getResource("/salaries1.csv").getPath();
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to use csvs when everywhere else in spark tests we create simple few row in memory datasets?

Copy link
Member Author

Choose a reason for hiding this comment

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

A mixture of "laziness and reproduction of the original issue". I wanted this test to be close to what the demo in nessie-demos looks like.

}

@Test
// Delta < 0.8 w/ Spark 2.x doesn't support multiple branches well (warnings when changing the configuration)
Copy link
Contributor

Choose a reason for hiding this comment

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

for my benefit can you explain a bit more? If i recall correctly 0.6.0 is Spark2 only and >0.7.0 is Spark3 only.

Copy link
Member Author

Choose a reason for hiding this comment

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

that's delta/spark2 - it complains with a warning that the configuration has changed and "might not have been applied completely".

.getOrCreate();
spark.sparkContext().setLogLevel("WARN");

nessieClient = NessieClient.builder().withUri(url).build();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can we rename the url parameter

@snazy snazy merged commit dc7e490 into projectnessie:main May 25, 2021
@snazy snazy deleted the delta-retry-on-conflict branch May 25, 2021 11: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