[PR1] refactoring :apps build and adding spark3.5/iceberg1.5 artifact #255

cbb330 · 2024-11-16T00:01:34Z

Summary

This is Part 1 of 2 for enabling compaction based MoR.

bring spark3.5 for creating delete files in testing
enable delete file compaction in spark3.1/spark3.5 and add unittests for such in spark3.5

problem: enabling Merge-on-Read for customer tables requires the ability to compact delete files. Compacting delete files is supported in spark3.1/iceberg1.2 but requires us to test it. We cannot test it, because creating delete files is not supported in spark3.1 (only compacting existing ones).

solution: we add spark3.5/iceberg1.5 compaction application, which extends the existing compaction test suite. in a follow up PR, we will add tests which create delete files and compacts the delete files under the latest versions.

Changes

Testing Done

Manually Tested on local docker setup. Please include commands ran, and their output.
Added new tests for the changes made.
Updated existing tests to reflect the changes made.
No tests added or updated. Please explain why. If unsure, please feel free to ask for help.
Some other form of testing like staging or soak time in production. Please explain.

all tests running on spark3.1 and spark3.5:

correct spark version resolutions for test runtime:

correct spark version resolutions for runtime:

Additional Information

Breaking Changes
Deprecations
Large PR broken into smaller PRs, and PR plan linked in the description.

For all the boxes checked, include additional details of the changes made in this pull request.

autumnust · 2024-11-19T00:13:25Z

Is this ready for review?

cbb330 · 2024-11-19T00:49:42Z

Is this ready for review?

@autumnust waiting on #221 to merge so i can rebase. this PR is forked from that.

if you want you can compare the commit delta here: https://github.com/linkedin/openhouse/pull/255/files/770a6d0497b9af7b1bedb876cbf639b71b3a9822..94c5100d5aa780a373ee5d0e14394ecdcec2f561

…sspath

jiang95-dev

Thanks for adding the new module! One major request is to use iceberg-convention-1.2.gradle and iceberg-convention-1.5.2.gradle to avoid duplicate code.

jiang95-dev · 2024-11-25T22:08:10Z

settings.gradle

@@ -25,6 +25,7 @@ pluginManagement {
 rootProject.name = 'openhouse'

 include ':apps:spark'
+include ':apps:spark-3.5'


Can we also rename this module?

when i attempt to rename :apps:spark-3.5, to :apps:openhouse-spark-apps-1.5_2.12

A problem occurred evaluating project ':apps:openhouse-spark-apps-1.5_2.12'. > Could not get unknown property 'sourceSets' for project ':apps:openhouse-spark-apps_2.12' of type org.gradle.api.Project.

which is... unexpected.I attempted a few different alternatives without success

jiang95-dev · 2024-11-25T22:08:43Z

libs/datalayout/build.gradle

@@ -1,7 +1,6 @@
 plugins {
    id 'openhouse.java-conventions'
    id 'openhouse.hadoop-conventions'
-    id 'openhouse.iceberg-conventions-1.2'


Why removing this?

this convention defines strictly 1.2. that strictly causes issue when used as a dependency in :apps:spark-3.5 which needs iceberg 1.5

we can also do:

replace this line with 1.5 conventions, since this can be pinned down by consumer :apps:spark-3.1. this is the same approach you used with services modules to 1.5 and overriding with strictly 1.2

exclude the transitive dependency from :apps:spark-3.5, but I would prefer not to do that and use the version resolver instead

compiling this module with spark 3.5/iceberg1.5 breaks :apps:spark3.1

but compiling this module with spark3.1/iceberg1.2 works in :apps:spark3.5 (although unsafe, but this is ok since it won't be productionized until next year)

so leaving this :libs:datalayout as a nonstrict 1.2 is preferred , which requires removing iceberg-conventions-1.2

jiang95-dev · 2024-11-25T22:09:55Z

buildSrc/src/main/groovy/openhouse.apps-spark-conventions.gradle

@@ -0,0 +1,123 @@
+plugins {


Nit. The name apps-spark-conventions doesn't sound right. Should apps-spark-commons be better?

jiang95-dev · 2024-11-25T22:11:28Z

apps/spark/build.gradle

@@ -1,38 +1,49 @@
 plugins {
-    id 'openhouse.java-conventions'
-    id 'openhouse.hadoop-conventions'
-    id 'openhouse.iceberg-conventions-1.2'


This line should not be removed.

jiang95-dev · 2024-11-25T22:12:42Z

apps/spark-3.5/build.gradle

+    implementation("org.apache.iceberg:iceberg-bundled-guava:${icebergVersion}")
+    implementation("org.apache.iceberg:iceberg-data:${icebergVersion}")
+    implementation("org.apache.iceberg:iceberg-core:${icebergVersion}")
+    implementation("org.apache.iceberg:iceberg-common:${icebergVersion}")


Add iceberg-conventions-1.5.2.gradle to remove them.

autumnust · 2024-11-26T02:37:44Z

libs/datalayout/build.gradle

+    sparkVersionSuffix = "3.1"
+    openhouseSparkRuntimeModule = ":integrations:spark:spark-${sparkVersionSuffix}:openhouse-spark-runtime_2.12"
+    tablesTestFixturesModule = ":tables-test-fixtures:tables-test-fixtures_2.12"


are changes in this file relevant to this PR?

yes, in this PR i must unpin 1.2 from the convention plugin removed above. i do this by removing convention plugin, and defining explicitly here and without any strictly version constraints.

this is so that consumers of this library (:apps) can override with their required version such as iceberg 1.5.2

autumnust · 2024-11-26T02:39:58Z

build.gradle

-        force 'com.fasterxml.jackson:jackson-bom:2.13.4'
-        force 'com.fasterxml.jackson.core:jackson-databind:2.13.4'


are these changes relevant ?

this may actually be dangerous to remove, and I will fix it, since idk what is currently relying on this.

but this strict version constraint breaks :apps:spark-3.5 because that also needs jackson 2.15

autumnust · 2024-11-26T02:41:37Z

apps/spark/build.gradle

+        version {
+            strictly("${icebergVersion}")
+        }


are these really helpful ? In practice I never find force or strictly covering everything that I want, since there could always be fatJar in the dependency Tree and these keywords don't work for them.

strictly here overrides any dependencies of compaction that are defining a higher version of 1.5.2, such as

services
cluster:storage
internalcatalog
htscatalog

without strictly, these dependencies will bump up iceberg in compactino unintentionally.

an alternative to forcing the version to be LOWER than what is defined in transitive dependencies, is:

excluding all iceberg stuff from all of the dependencies above. but i think this is less readable and from what I understand, utilizing the resolution rules is preferred to exclusions

using the 1.2 conventions plugin which ALSO defines strictly. but i prefer to be explicit

autumnust · 2024-11-26T02:44:24Z

integrations/spark/spark-3.5/openhouse-spark-runtime/build.gradle

@@ -55,7 +55,9 @@ dependencies {
  fatJarPackagedDependencies(project(path: ':integrations:spark:spark-3.1:openhouse-spark-runtime_2.12', configuration: 'shadow')) {
    transitive = false
  }
-  implementation(project(path: ':integrations:spark:spark-3.1:openhouse-spark-runtime_2.12', configuration: 'shadow'))
+  implementation(project(path: ':integrations:spark:spark-3.1:openhouse-spark-runtime_2.12', configuration: 'shadow')) {
+    exclude group: "org.apache.iceberg", module: "iceberg-spark-runtime-3.1_2.12"


I thought this one is shaded already?

well, 3.5 was being used regardless. but i noticed both 3.1 and 3.5 were available on classpath before this exclusion, i don't think this was intentional, so i'm cleaning this up here.

cbb330 force-pushed the main branch 7 times, most recently from 7dcde77 to 3e7e046 Compare November 18, 2024 01:48

cbb330 changed the title ~~WIP: [GA Merge-on-Read] enabling compaction for MoR related delete files~~ WIP: [GA Merge-on-Read] refactoring :apps build and adding spark3.5/iceberg1.5 artifact Nov 18, 2024

cbb330 closed this Nov 18, 2024

cbb330 force-pushed the main branch from 3e7e046 to 071c2b0 Compare November 18, 2024 03:19

cbb330 reopened this Nov 18, 2024

cbb330 force-pushed the main branch from 0a30706 to 94c5100 Compare November 18, 2024 21:10

cbb330 force-pushed the main branch 2 times, most recently from 81e11a0 to ab39faa Compare November 24, 2024 03:13

working apps spark3.5 and spark 3.1 build and test

3ce4f22

cbb330 force-pushed the main branch from ab39faa to 3ce4f22 Compare November 24, 2024 03:15

excluding 3.1 runtime dependency for 3.5 that shouldn't be on the cla…

7cee1ed

…sspath

cbb330 changed the title ~~WIP: [GA Merge-on-Read] refactoring :apps build and adding spark3.5/iceberg1.5 artifact~~ [GA Merge-on-Read] refactoring :apps build and adding spark3.5/iceberg1.5 artifact Nov 24, 2024

jiang95-dev requested changes Nov 25, 2024

View reviewed changes

autumnust requested changes Nov 26, 2024

View reviewed changes

sumedhsakdeo requested a review from teamurko November 26, 2024 05:00

sumedhsakdeo changed the title ~~[GA Merge-on-Read] refactoring :apps build and adding spark3.5/iceberg1.5 artifact~~ [PR1] refactoring :apps build and adding spark3.5/iceberg1.5 artifact Nov 26, 2024

cbb330 added 3 commits November 26, 2024 16:38

using conventions instead of explicit iceberg

415deb8

renaming convetions to commons

339cffd

renaming module

08146aa

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PR1] refactoring :apps build and adding spark3.5/iceberg1.5 artifact #255

[PR1] refactoring :apps build and adding spark3.5/iceberg1.5 artifact #255

cbb330 commented Nov 16, 2024 •

edited

Loading

autumnust commented Nov 19, 2024

cbb330 commented Nov 19, 2024 •

edited

Loading

jiang95-dev left a comment

jiang95-dev Nov 25, 2024

cbb330 Nov 27, 2024 •

edited

Loading

jiang95-dev Nov 25, 2024

cbb330 Nov 26, 2024 •

edited

Loading

cbb330 Nov 28, 2024

jiang95-dev Nov 25, 2024

jiang95-dev Nov 25, 2024

jiang95-dev Nov 25, 2024

autumnust Nov 26, 2024

cbb330 Nov 26, 2024

autumnust Nov 26, 2024

cbb330 Nov 26, 2024

autumnust Nov 26, 2024

cbb330 Nov 26, 2024 •

edited

Loading

autumnust Nov 26, 2024

cbb330 Nov 26, 2024 •

edited

Loading

		force 'com.fasterxml.jackson:jackson-bom:2.13.4'
		force 'com.fasterxml.jackson.core:jackson-databind:2.13.4'

[PR1] refactoring :apps build and adding spark3.5/iceberg1.5 artifact #255

Are you sure you want to change the base?

[PR1] refactoring :apps build and adding spark3.5/iceberg1.5 artifact #255

Conversation

cbb330 commented Nov 16, 2024 • edited Loading

Summary

Changes

Testing Done

Additional Information

autumnust commented Nov 19, 2024

cbb330 commented Nov 19, 2024 • edited Loading

jiang95-dev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbb330 Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbb330 Nov 26, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbb330 Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbb330 Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

cbb330 commented Nov 16, 2024 •

edited

Loading

cbb330 commented Nov 19, 2024 •

edited

Loading

cbb330 Nov 27, 2024 •

edited

Loading

cbb330 Nov 26, 2024 •

edited

Loading

cbb330 Nov 26, 2024 •

edited

Loading

cbb330 Nov 26, 2024 •

edited

Loading