[1094] Fix config for S3 multi-cluster mode #1095

scottsand-db · 2022-04-22T17:11:12Z

Problem Description

problem 1: there was a mismatch between the config params in the documentation (io.delta.storage.S3DynamoDBLogStore.ddb.tableName) and the config params actually used in the code + in the integration test (io.delta.storage.ddb.tableName).
solution 1: include the S3DynamoDBLogStore string in the config in the code.
problem 2: the io.delta.storage prefix didn't work when specified using --conf. they DID work using spark.conf.set() or SparkSession.builder.config() but not with --conf.
solution 2: we now allow 2 prefixes.
- spark.io.delta.storage.S3DynamoDBLogStore.$key..... this will work all contexts (--conf, spark.conf.set(), etc).
- io.delta.storage.S3DynamoDBLogStore.$key. this is the original prefix. this will be able to be used by delta-standalone and flink and hive. they just use hadoopConfs and don't need to have prefix starting with spark

PR Description

resolves [BUG] issues configuring AWS S3 multi-cluster mode #1094
update config for S3 multi-cluster mode (i.e. S3DynamoDBLogStore) to match the public documentation. the configs were missing the string literal S3DynamoDBLogStore from the conf prefix
now supports 2 confs. spark.io.delta.storage.S3DynamoDBLogStore.$key and io.delta.storage.S3DynamoDBLogStore.$key.

How was this patch tested?

added unit test for the 2 new confs
ran integration test using existing + new s3 table, and using existing + new ddb table
ran manual tests using locally published jars on pyspark + spark-shell + spark-submit (spark-submit via integration test)
tested using --conf as well as spark.conf.set as well as SparkSession.builder.config()

Does this PR introduce any user-facing changes?

Not really, just fixes a just-released, experimental LogStore config key

- added test case - fix mispelled variable

scottsand-db · 2022-04-22T22:48:28Z

storage-s3-dynamodb/integration_tests/dynamodb_logstore.py

@@ -45,7 +45,7 @@
 export DELTA_NUM_ROWS=16

 ./run-integration-tests.py --run-storage-s3-dynamodb-integration-tests \
-    --dbb-packages org.apache.hadoop:hadoop-aws:3.3.1,com.amazonaws:aws-java-sdk-bundle:1.12.142 \
+    --dbb-packages org.apache.hadoop:hadoop-aws:3.3.1 \


com.amazonaws:aws-java-sdk-bundle:1.12.142 is a compile dependency for org.apache.hadoop:hadoop-aws:3.3.1

scottsand-db · 2022-04-22T22:48:52Z

storage-s3-dynamodb/integration_tests/dynamodb_logstore.py

+    .config("spark.delta.logStore.s3.impl", delta_storage) \
+    .config("spark.delta.logStore.s3a.impl", delta_storage) \
+    .config("spark.delta.logStore.s3n.impl", delta_storage) \


let's idiot-proof this to work with s3, s3a, s3n

scottsand-db · 2022-04-22T22:49:14Z

storage-s3-dynamodb/src/main/java/io/delta/storage/S3DynamoDBLogStore.java

     */
-    private static final String CONF_PREFIX = "io.delta.storage.";
-    private static final String DBB_CLIENT_TABLE = "ddb.tableName";
-    private static final String DBB_CLIENT_REGION = "ddb.region";


renamed from DBB to DDB :)

scottsand-db · 2022-04-25T15:51:20Z

Test Failed

https://github.com/delta-io/delta/runs/6135959050?check_suite_focus=true

[info] org.apache.spark.sql.delta.SnapshotManagementSuite *** ABORTED *** (19 seconds, 956 milliseconds)
[info]   The code passed to eventually never returned normally. Attempted 15 times over 10.012527203 seconds. Last failure message: There are 1 possibly leaked file streams.. (SharedSparkSession.scala:164)
[info]   org.scalatest.exceptions.TestFailedDueToTimeoutException:
[info]   at org.scalatest.enablers.Retrying$$anon$4.tryTryAgain$2(Retrying.scala:189)
[info]   at org.scalatest.enablers.Retrying$$anon$4.retry(Retrying.scala:196)
[info]   at org.scalatest.concurrent.Eventually.eventually(Eventually.scala:313)
[info]   at org.scalatest.concurrent.Eventually.eventually$(Eventually.scala:312)
[info]   at org.apache.spark.sql.delta.SnapshotManagementSuite.eventually(SnapshotManagementSuite.scala:31)
[info]   at org.apache.spark.sql.test.SharedSparkSessionBase.afterEach(SharedSparkSession.scala:164)
[info]   at org.apache.spark.sql.test.SharedSparkSessionBase.afterEach$(SharedSparkSession.scala:158)
[info]   at org.apache.spark.sql.delta.SnapshotManagementSuite.afterEach(SnapshotManagementSuite.scala:31)
[info]   at org.scalatest.BeforeAndAfterEach.$anonfun$runTest$1(BeforeAndAfterEach.scala:247)

scottsand-db · 2022-04-25T16:40:40Z

core/src/test/scala/org/apache/spark/sql/delta/LogStoreSuite.scala

-  test("java LogStore is instantiated with hadoopConf with SQLConf values") {
-    val sparkConf = new SparkConf()
-      .setMaster("local")
-      // equivalent to --conf spark.delta.storage.custom.key=foo


turns out this just is not true

tdas · 2022-04-25T20:27:26Z

storage-s3-dynamodb/src/test/scala/io/delta/storage/ExternalLogStoreSuite.scala

+    val basePrefixKey = "io.delta.storage.S3DynamoDBLogStore.ddb.tableName"
+
+    // Sanity check
+    assert(sparkPrefixKey == SPARK_CONF_PREFIX + "." + DDB_CLIENT_TABLE)


nit: typically these should be require as this failure does not mean some thing is wrong in the production code... this failure means something is wrong in the test code.

- problem 1: there was a mismatch between the config params in the documentation (`io.delta.storage.S3DynamoDBLogStore.ddb.tableName`) and the config params actually used in the code + in the integration test (`io.delta.storage.ddb.tableName`). - solution 1: include the `S3DynamoDBLogStore` string in the config in the code. - problem 2: the `io.delta.storage` prefix didn't work when specified using `--conf`. they DID work using `spark.conf.set()` or `SparkSession.builder.config()` but not with `--conf`. - solution 2: we now allow 2 prefixes. - `spark.io.delta.storage.S3DynamoDBLogStore.$key.....` this will work all contexts (`--conf`, `spark.conf.set()`, etc). - `io.delta.storage.S3DynamoDBLogStore.$key`. this is the original prefix. this will be able to be used by delta-standalone and flink and hive. they just use hadoopConfs and don't need to have prefix starting with `spark` - resolves delta-io#1094 - update config for S3 multi-cluster mode (i.e. S3DynamoDBLogStore) to match the public documentation. the configs were missing the string literal `S3DynamoDBLogStore` from the conf prefix - now supports 2 confs. `spark.io.delta.storage.S3DynamoDBLogStore.$key` and `io.delta.storage.S3DynamoDBLogStore.$key`. - added unit test for the 2 new confs - ran integration test using existing + new s3 table, and using existing + new ddb table - ran manual tests using locally published jars on pyspark + spark-shell + spark-submit (spark-submit via integration test) - tested using `--conf` as well as `spark.conf.set` as well as `SparkSession.builder.config()` Not really, just fixes a just-released, experimental LogStore config key Closes delta-io#1095 Signed-off-by: Scott Sandre <[email protected]> GitOrigin-RevId: e5db1e6b0dfe958e3234644462891f269313ca33

- problem 1: there was a mismatch between the config params in the documentation (`io.delta.storage.S3DynamoDBLogStore.ddb.tableName`) and the config params actually used in the code + in the integration test (`io.delta.storage.ddb.tableName`). - solution 1: include the `S3DynamoDBLogStore` string in the config in the code. - problem 2: the `io.delta.storage` prefix didn't work when specified using `--conf`. they DID work using `spark.conf.set()` or `SparkSession.builder.config()` but not with `--conf`. - solution 2: we now allow 2 prefixes. - `spark.io.delta.storage.S3DynamoDBLogStore.$key.....` this will work all contexts (`--conf`, `spark.conf.set()`, etc). - `io.delta.storage.S3DynamoDBLogStore.$key`. this is the original prefix. this will be able to be used by delta-standalone and flink and hive. they just use hadoopConfs and don't need to have prefix starting with `spark` - resolves #1094 - update config for S3 multi-cluster mode (i.e. S3DynamoDBLogStore) to match the public documentation. the configs were missing the string literal `S3DynamoDBLogStore` from the conf prefix - now supports 2 confs. `spark.io.delta.storage.S3DynamoDBLogStore.$key` and `io.delta.storage.S3DynamoDBLogStore.$key`. - added unit test for the 2 new confs - ran integration test using existing + new s3 table, and using existing + new ddb table - ran manual tests using locally published jars on pyspark + spark-shell + spark-submit (spark-submit via integration test) - tested using `--conf` as well as `spark.conf.set` as well as `SparkSession.builder.config()` Not really, just fixes a just-released, experimental LogStore config key Closes #1095 Signed-off-by: Scott Sandre <[email protected]> GitOrigin-RevId: e5db1e6b0dfe958e3234644462891f269313ca33

## Problem Description - problem 1: there was a mismatch between the config params in the documentation (`io.delta.storage.S3DynamoDBLogStore.ddb.tableName`) and the config params actually used in the code + in the integration test (`io.delta.storage.ddb.tableName`). - solution 1: include the `S3DynamoDBLogStore` string in the config in the code. - problem 2: the `io.delta.storage` prefix didn't work when specified using `--conf`. they DID work using `spark.conf.set()` or `SparkSession.builder.config()` but not with `--conf`. - solution 2: we now allow 2 prefixes. - `spark.io.delta.storage.S3DynamoDBLogStore.$key.....` this will work all contexts (`--conf`, `spark.conf.set()`, etc). - `io.delta.storage.S3DynamoDBLogStore.$key`. this is the original prefix. this will be able to be used by delta-standalone and flink and hive. they just use hadoopConfs and don't need to have prefix starting with `spark` ## PR Description - resolves delta-io#1094 - update config for S3 multi-cluster mode (i.e. S3DynamoDBLogStore) to match the public documentation. the configs were missing the string literal `S3DynamoDBLogStore` from the conf prefix - now supports 2 confs. `spark.io.delta.storage.S3DynamoDBLogStore.$key` and `io.delta.storage.S3DynamoDBLogStore.$key`. - added unit test for the 2 new confs - ran integration test using existing + new s3 table, and using existing + new ddb table - ran manual tests using locally published jars on pyspark + spark-shell + spark-submit (spark-submit via integration test) - tested using `--conf` as well as `spark.conf.set` as well as `SparkSession.builder.config()` ## Does this PR introduce _any_ user-facing changes? Not really, just fixes a just-released, experimental LogStore config key Closes delta-io#1095 Signed-off-by: Scott Sandre <[email protected]> GitOrigin-RevId: e5db1e6b0dfe958e3234644462891f269313ca33

updated config keys and integration test

7324f62

scottsand-db self-assigned this Apr 22, 2022

scottsand-db added 6 commits April 22, 2022 10:53

Update dynamodb_logstore.py

f15e6a5

delete test from from LogStoreSuite

e7a6d3a

added two options for config prefix: io.delta... or spark.io.delta...

4d0edcf

- added test case - fix mispelled variable

fix import order

33b94d3

whitespace change in logInfo msg

cf01ee1

Update dynamodb_logstore.py

d265b8f

scottsand-db changed the title ~~[WIP - NEED TO QA] [1094] Fix config for S3 multi-cluster mode~~ [1094] Fix config for S3 multi-cluster mode Apr 22, 2022

scottsand-db requested review from tdas and vkorukanti April 22, 2022 22:40

scottsand-db commented Apr 22, 2022

View reviewed changes

scottsand-db commented Apr 25, 2022

View reviewed changes

tdas reviewed Apr 25, 2022

View reviewed changes

tdas approved these changes Apr 25, 2022

View reviewed changes

scottsand-db added the waiting for merge label Apr 25, 2022

scottsand-db closed this in 6905ce7 Apr 26, 2022

vkorukanti added this to the 2.0 milestone Jun 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1094] Fix config for S3 multi-cluster mode #1095

[1094] Fix config for S3 multi-cluster mode #1095

scottsand-db commented Apr 22, 2022 •

edited

Loading

scottsand-db Apr 22, 2022

scottsand-db Apr 22, 2022

tdas Apr 25, 2022

scottsand-db Apr 22, 2022

scottsand-db commented Apr 25, 2022

scottsand-db Apr 25, 2022

tdas Apr 25, 2022

[1094] Fix config for S3 multi-cluster mode #1095

[1094] Fix config for S3 multi-cluster mode #1095

Conversation

scottsand-db commented Apr 22, 2022 • edited Loading

Problem Description

PR Description

How was this patch tested?

Does this PR introduce any user-facing changes?

scottsand-db Apr 22, 2022

Choose a reason for hiding this comment

scottsand-db Apr 22, 2022

Choose a reason for hiding this comment

tdas Apr 25, 2022

Choose a reason for hiding this comment

scottsand-db Apr 22, 2022

Choose a reason for hiding this comment

scottsand-db commented Apr 25, 2022

scottsand-db Apr 25, 2022

Choose a reason for hiding this comment

tdas Apr 25, 2022

Choose a reason for hiding this comment

scottsand-db commented Apr 22, 2022 •

edited

Loading