Allow impl for LocationProvider for to be created during runtime #1531

mickjermsurawong-stripe · 2020-09-30T04:59:17Z

This PR allows users to supply implementation of LocationProvider to be created during runtime.
This is motivated from the proposal to solve data localization.
From the community sync, the feedback is to make to leave implementation to users so other use cases outside data localization could make use of this too

jackye1995 · 2020-10-01T02:01:03Z

I have 2 concerns with this PR:

I think we should in addition propose dynamically loading the LocationProvider implementation, instead of using a boolean to switch on or off each implementation. This will allow any user to easily plugin a custom implementation. I wonder if such proposal is enough to both satisfy your need and also keep the community version general.
for this specific implementation, the approach of using table property to get localizedDataLocationLookUp map feels not scalable to me if we want to make it a useful generic implementation. For example, customers might want to have 1 HDFS cluster per AWS local zone to read and write data due to latency requirement, and this setup easily generate hundreds to thousands of table properties. I don't know if there is any bad impact for such use case, since table properties is not designed to hold a lot of data.

mickjermsurawong-stripe · 2020-10-01T05:08:42Z

hi @jackye1995 thanks for the feedback here! This is aligned with the discussion we had during the sync for solution that folks can easily customizable their locations. I've re-purposed this PR to address this. If this is in, we can easily implement our data localization here.

jackye1995 · 2020-10-01T05:22:18Z

Thank you! I created an issue for this, can you link it to this PR? #1538

mickjermsurawong-stripe · 2020-10-01T17:02:40Z

cc @stripe/iceberg-reviewers, I iterated on this based on feedback to have pluggable impl for location provider instead

rdblue · 2020-10-02T20:11:56Z

Thanks for working on this, @mickjermsurawong-stripe!

I like dynamically loading a location provider, and I like that the changes required are pretty small. That looks good to me.

What I'd like to change is how this is tested. There are two main issues with it. First, the tests are in an unrelated suite for Spark that is mainly testing that Spark works with a variety of different schemas. It doesn't seem appropriate to put the tests for injecting a location provider in a different module and into this suite. Second, the TestLocationProvider returns real paths and the tests work by writing data files and validating that files are underneath those paths. That does test that it works, but I think this makes validation much harder. This approach requires a lot of test code and runs a lot of unrelated cases through to validate the location provider.

I would prefer a more targeted test in a new suited in core, TestLocationProvider. That would have a case that injects a TestLocationProvider that keeps track of calls in static variables, so you'd be able to inject the provider, get it from table.operations().locationProvider(), call some methods, and validate that TestLocationProvider received the calls.

We also need negative tests for when the provider can't be found. I'd like to have a good error message like "Cannot find location provider class: %s".

rdblue · 2020-10-02T22:37:13Z

@jackye1995, I think this dynamic loading approach might be a good idea for injecting S3FileIO as well. What do you think?

jackye1995 · 2020-10-02T22:47:13Z

@rdblue yes, that is exactly how I implement, just waiting for your branch to cross check all details about S3FileIO before opening a PR

mickjermsurawong-stripe · 2020-10-03T05:07:43Z

@rdblue, ah thank you for the feedback on test! I guess the complicated test here is the artifacts of me trying to test the logic in my first data localization feature.

I created a separate test as suggested; I made use of the existing TableTestBase and construct tests based on table.locationProvider() instead of its underlying operations (though that also directly delegates to the ops). I also addressed feedback on error message.

rdblue · 2020-10-05T17:11:43Z

core/src/test/java/org/apache/iceberg/TestLocationProvider.java

+        String.format("Unable to instantiate the provided implementation %s for %s.",
+            nonExistentImpl,
+            LocationProvider.class));
+    this.table.locationProvider();


Instead of exception rules, we like to use AssertHelpers.assertThrows. That way, the test doesn't need to end when the exception is thrown and you can run other validations.

Also, we don't typically use the prefix this. unless setting an instance field, unless it helps understanding what's going on. We always using when assigning, though.

rdblue · 2020-10-05T17:12:54Z

core/src/test/java/org/apache/iceberg/TestLocationProvider.java

+
+  // publicly visible for testing to be dynamically loaded
+  public static class InvalidDynamicallyLoadedLocationProvider implements LocationProvider {
+    // No public constructor


Should we support a no-arg constructor as well as the one that passes the table location and properties?

rdblue · 2020-10-05T17:13:13Z

core/src/test/java/org/apache/iceberg/TestLocationProvider.java

+  }
+
+  @Test
+  public void testInvalidDynamicallyLoadedLocationProvider() {


What happens if the class doesn't implement LocationProvider?

Could you add a test case for that?

aokolnychyi · 2020-10-05T17:14:37Z

core/src/main/java/org/apache/iceberg/TableProperties.java

@@ -87,6 +87,8 @@ private TableProperties() {
  public static final String OBJECT_STORE_ENABLED = "write.object-storage.enabled";
  public static final boolean OBJECT_STORE_ENABLED_DEFAULT = false;

+  public static final String LOCATION_PROVIDER_IMPL = "write.location-provider.impl";


Do we want to add this to the docs?

rdblue · 2020-10-05T21:08:10Z

core/src/main/java/org/apache/iceberg/LocationProviders.java

+        DynConstructors.builder(LocationProvider.class)
+            .impl(impl).build()
+    );
+    Optional<DynConstructors.Ctor<LocationProvider>> twoArgConstructor = Optional.empty();


When the arguments passed to one constructor are a prefix of the args passed to the other, there is no need to use a different builder or call because the arguments actually passed are limited by the number of parameters defined by the constructor. That makes it possible to pass both arguments to either one, and any extras are ignored.

That helps simplify cases like this because you don't need the extra optional logic:

ctor = DynConstructors.builder(LocationProvider.class) .impl(impl, String.class, Map.class) .impl(impl) // fall back to no-arg constructor .build() return ctor.newInstance(location, properties);

thanks for the explanation here

rdblue · 2020-10-05T21:09:19Z

site/docs/javadoc/master/constant-values.html

 <td class="colFirst"><a name="org.apache.iceberg.TableProperties.WRITE_METADATA_LOCATION">
 <!--   -->
 </a><code>public&nbsp;static&nbsp;final&nbsp;java.lang.String</code></td>
 <td><code><a href="org/apache/iceberg/TableProperties.html#WRITE_METADATA_LOCATION">WRITE_METADATA_LOCATION</a></code></td>
 <td class="colLast"><code>"write.metadata.path"</code></td>
 </tr>
-<tr class="altColor">
+<tr class="rowColor">


Can you remove the html changes please? The docs that @aokolnychyi was referring to are built from markdown source located in site/docs/configuration.md.

rdblue · 2020-10-05T21:11:55Z

Thanks for the update, @mickjermsurawong-stripe! I think the implementation can be quite a bit simpler and more like the original, even with support for the no-arg constructor. Sorry I didn't mention that earlier, before you went ahead and implemented it this way.

mickjermsurawong-stripe · 2020-10-05T21:38:55Z

core/src/main/java/org/apache/iceberg/LocationProviders.java

+                "has a public no-arg constructor or a two-arg constructor " +
+                "taking in the string base table location and its property string map.",
+            impl, LocationProvider.class));
+      }


If provided class has the specified constructor signature, but not implementing LocationProvider, it can still be loaded into the dynConstructor. The following block differentiates this missing interface more explicitly.
Might be a bit more code churn, but it does give more signals to users.

mickjermsurawong-stripe · 2020-10-05T22:47:54Z

thanks @rdblue for the review so far! I believe this should be ready for another review.

rdblue · 2020-10-05T23:08:48Z

core/src/main/java/org/apache/iceberg/LocationProviders.java

+      } catch (ClassCastException e) {
+        throw new IllegalArgumentException(
+            String.format("Provided implementation for dynamic instantiation should implement %s, " +
+                "but found dynamic constructor %s.", LocationProvider.class, ctor));


Dynamic constructor?

Here as well you should probably throw the exception with the original as a cause so the context isn't lost.

rdblue · 2020-10-05T23:11:58Z

core/src/main/java/org/apache/iceberg/LocationProviders.java

+                "Make sure the implementation is in classpath, and that it either " +
+                "has a public no-arg constructor or a two-arg constructor " +
+                "taking in the string base table location and its property string map.",
+            impl, LocationProvider.class));


Since you're catching the exception, it probably makes more sense to use buildChecked so the exception class isn't generic. That exception also has good information about why all of the implementations failed, so I would recommend adding that exception as a cause to this one that is thrown. I like throwing this one for context, though!

mickjermsurawong-stripe · 2020-10-05T23:35:33Z

Propagating the causes now should give stacktrace for these two cases, with more specific causes.

 java.lang.IllegalArgumentException: Unable to find a constructor for implementation org.apache.iceberg.TestLocationProvider$InvalidArgTypesDynamicallyLoadedLocationProvider of interface org.apache.iceberg.io.LocationProvider. Make sure the implementation is in classpath, and that it either has a public no-arg constructor or a two-arg constructor taking in the string base table location and its property string map.
        at org.apache.iceberg.LocationProviders.locationsFor(LocationProviders.java:48)
        at org.apache.iceberg.TestTables$TestTableOperations.locationProvider(TestTables.java:212)
        at org.apache.iceberg.BaseTable.locationProvider(BaseTable.java:199)
        at org.apache.iceberg.TestLocationProvider.testInvalidArgTypesDynamicallyLoadedLocationProvider(TestLocationProvider.java:208)

        Caused by:
        java.lang.NoSuchMethodException: Cannot find constructor for interface org.apache.iceberg.io.LocationProvider
                Missing org.apache.iceberg.TestLocationProvider$InvalidArgTypesDynamicallyLoadedLocationProvider() [java.lang.NoSuchMethodException: org.apache.iceberg.TestLocationProvider$InvalidArgTypesDynamicallyLoadedLocationProvider.<init>()]
                Missing org.apache.iceberg.TestLocationProvider$InvalidArgTypesDynamicallyLoadedLocationProvider(java.lang.String,java.util.Map) [java.lang.NoSuchMethodException: org.apache.iceberg.TestLocationProvider$InvalidArgTypesDynamicallyLoadedLocationProvider.<init>(java.lang.String, java.util.Map)]

and

java.lang.IllegalArgumentException: Provided implementation for dynamic instantiation should implement interface org.apache.iceberg.io.LocationProvider.
        at org.apache.iceberg.LocationProviders.locationsFor(LocationProviders.java:59)
        at org.apache.iceberg.TestTables$TestTableOperations.locationProvider(TestTables.java:212)
        at org.apache.iceberg.BaseTable.locationProvider(BaseTable.java:199)
        at org.apache.iceberg.TestLocationProvider.testInvalidNoInterfaceDynamicallyLoadedLocationProvider(TestLocationProvider.java:190)

        Caused by:
        java.lang.ClassCastException: org.apache.iceberg.TestLocationProvider$InvalidNoInterfaceDynamicallyLoadedLocationProvider cannot be cast to org.apache.iceberg.io.LocationProvider
            at org.apache.iceberg.LocationProviders.locationsFor(LocationProviders.java:56)
            ... 3 more

rdblue · 2020-10-06T02:39:21Z

Thanks, @mickjermsurawong-stripe! I merged this. Nice work, and thank you for your patience and quick turn-around with the review comments!

mickjermsurawong-stripe · 2020-10-06T02:51:53Z

Thank you Ryan! This will help with our internal feature :)

mickjermsurawong-stripe added 4 commits September 29, 2020 13:08

new table properties

db7f3e5

expose helper method to get class from partition field

4074a94

localized location provider

9412875

test dataframe writes

ad8b9c1

probot-autolabeler bot added API core spark labels Sep 30, 2020

mickjermsurawong-stripe added 2 commits September 29, 2020 22:25

formatting

9b5f96f

format tests

51ced5f

mickjermsurawong-stripe changed the title ~~Localized store based on partition~~ [wip] Localized store based on partition Sep 30, 2020

jackye1995 mentioned this pull request Oct 1, 2020

Dynamically load LocationProvider implementation #1538

Closed

mickjermsurawong-stripe added 4 commits September 30, 2020 21:54

dynamically load location provider

c0abcf6

revert refactoring

bd2d8af

remove extra line

a5eff74

rename test

706cfcb

mickjermsurawong-stripe changed the title ~~[wip] Localized store based on partition~~ Allow impl for LocationProvider for to be created during runtime Oct 1, 2020

mickjermsurawong-stripe added 5 commits October 2, 2020 20:57

address feedback: test location provider

2c51cd7

revert old test

34192da

improve error message

77210d3

format test

73fe85d

propagate exception

9b22a8c

rdblue reviewed Oct 5, 2020

View reviewed changes

aokolnychyi reviewed Oct 5, 2020

View reviewed changes

mickjermsurawong-stripe added 7 commits October 5, 2020 12:00

support no-arg and write more negative tests

c0c25a2

use assert helper for future test extension

6f60d5f

order and update python props

c14078a

prefix with new prop with WRITE

a34b5d2

update docs

299ed00

add doc stiring

0eee288

fix formatting

5906be0

rdblue reviewed Oct 5, 2020

View reviewed changes

mickjermsurawong-stripe added 3 commits October 5, 2020 14:35

simplify constructor

5f9be8c

add doc to new property

3aeb9e5

revert manual html updates

2729b79

mickjermsurawong-stripe commented Oct 5, 2020

View reviewed changes

rdblue reviewed Oct 5, 2020

View reviewed changes

propagate cause

8c324c5

rdblue linked an issue Oct 5, 2020 that may be closed by this pull request

Dynamically load LocationProvider implementation #1538

Closed

rdblue merged commit 61aa677 into apache:master Oct 6, 2020

jackye1995 mentioned this pull request Nov 11, 2020

Support relative paths in Table Metadata #1617

Open

rdblue added this to the Java 0.10.0 Release milestone Nov 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow impl for LocationProvider for to be created during runtime #1531

Allow impl for LocationProvider for to be created during runtime #1531

mickjermsurawong-stripe commented Sep 30, 2020 •

edited

Loading

jackye1995 commented Oct 1, 2020

mickjermsurawong-stripe commented Oct 1, 2020

jackye1995 commented Oct 1, 2020

mickjermsurawong-stripe commented Oct 1, 2020

rdblue commented Oct 2, 2020

rdblue commented Oct 2, 2020

jackye1995 commented Oct 2, 2020

mickjermsurawong-stripe commented Oct 3, 2020

rdblue Oct 5, 2020

rdblue Oct 5, 2020

rdblue Oct 5, 2020

aokolnychyi Oct 5, 2020

rdblue Oct 5, 2020

rdblue Oct 5, 2020

mickjermsurawong-stripe Oct 5, 2020

rdblue Oct 5, 2020 •

edited

Loading

mickjermsurawong-stripe Oct 5, 2020

rdblue commented Oct 5, 2020

mickjermsurawong-stripe Oct 5, 2020 •

edited

Loading

mickjermsurawong-stripe commented Oct 5, 2020

rdblue Oct 5, 2020

rdblue Oct 5, 2020

rdblue Oct 5, 2020

mickjermsurawong-stripe commented Oct 5, 2020 •

edited

Loading

rdblue commented Oct 6, 2020

mickjermsurawong-stripe commented Oct 6, 2020

Allow impl for LocationProvider for to be created during runtime #1531

Allow impl for LocationProvider for to be created during runtime #1531

Conversation

mickjermsurawong-stripe commented Sep 30, 2020 • edited Loading

jackye1995 commented Oct 1, 2020

mickjermsurawong-stripe commented Oct 1, 2020

jackye1995 commented Oct 1, 2020

mickjermsurawong-stripe commented Oct 1, 2020

rdblue commented Oct 2, 2020

rdblue commented Oct 2, 2020

jackye1995 commented Oct 2, 2020

mickjermsurawong-stripe commented Oct 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdblue Oct 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdblue commented Oct 5, 2020

mickjermsurawong-stripe Oct 5, 2020 • edited Loading

Choose a reason for hiding this comment

mickjermsurawong-stripe commented Oct 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickjermsurawong-stripe commented Oct 5, 2020 • edited Loading

rdblue commented Oct 6, 2020

mickjermsurawong-stripe commented Oct 6, 2020

mickjermsurawong-stripe commented Sep 30, 2020 •

edited

Loading

rdblue Oct 5, 2020 •

edited

Loading

mickjermsurawong-stripe Oct 5, 2020 •

edited

Loading

mickjermsurawong-stripe commented Oct 5, 2020 •

edited

Loading