Build plugin with platform specific JDK #69

akshayrai · 2021-04-19T20:34:42Z

Motivation:
As part of the Trino upgrade/migration (#66), we need to isolate and build transport modules for different engines with different jdk versions. For instance, Spark & Hive needs to build with jdk8 and Trino has a build requirement on jdk11.

Verified that the compiled classes are targeted for the respective jdks using javap -verbose <class>

shardulm94 · 2021-04-20T07:44:29Z

transportable-udfs-examples/transportable-udfs-example-udfs/build.gradle

@@ -7,18 +7,26 @@ buildscript {
 apply plugin: 'java'
 apply plugin: 'com.linkedin.transport.plugin'

+println 'Running build for Hive, Presto & Spark'
+transport.engines = ['hive', 'presto', 'spark']


This put the onus on the client to decide which engines to run with Java 8 and which to run with Java 11. However the client is currently abstracted away from the engine version numbers, so I don't think the decision about toolchains should be upto the client. I think Transport should be handling this within the TransportPlugin.

Agreed! but I am not sure if it is possible to control the jdk version from inside the plugin. I will explore the option you suggested below. If that works, it would be the best way forward.

shardulm94 · 2021-04-20T07:46:14Z

transportable-udfs-plugin/src/main/java/com/linkedin/transport/plugin/TransportPlugin.java

+      }
+
+      // Isolate engine/platform dependencies
+      extension.engines.forEach(engine -> {


This means that the user will have to run two build for every project, one build for Trino and a second build for Spark, Hive. I would prefer something which does not involve such special handling in user code.

Gradle allows setting toolchains on a task level. I think we should modify TransportPlugin to set Java 11 toolchain for Trino sourceset's compileJava and test tasks. With this, we can handle everything within the same Gradle build.

Interesting! thanks for the pointer. I will take a further look if this is feasible.

akshayrai · 2021-04-24T00:49:11Z

Thanks @shardulm94 for helping debug and review the PR!

shardulm94

Thanks @akshayrai! We are almost good to commit this. Just some minor comments.

shardulm94 · 2021-04-24T01:15:33Z

transportable-udfs-examples/transportable-udfs-example-udfs/build.gradle

-plugins.withId('com.github.hierynomus.license') {
-  licenseHive.enabled = false
-  licensePresto.enabled = false
-  licenseSpark.enabled = false
+tasks.whenTaskAdded { task ->
+  if (task.name.startsWith("licensePresto")
+      || task.name.startsWith("licenseHive")
+      || task.name.startsWith("licenseSpark")) {
+    task.enabled = false
+  }
 }


I guess this change is no longer needed?

shardulm94 · 2021-04-24T01:19:25Z

transportable-udfs-plugin/src/main/java/com/linkedin/transport/plugin/TransportPlugin.java

-    project.getTasks()
-        .named(outputSourceSet.getCompileTaskName(platform.getLanguage().toString()))
-        .configure(task -> task.dependsOn(generateWrappersTask));


Can we leave this as is and then replace the switch block below with if (platform.getLanguage() = JAVA)?

shardulm94 · 2021-04-24T01:20:39Z

transportable-udfs-plugin/src/main/java/com/linkedin/transport/plugin/TransportPlugin.java

+            .named(outputSourceSet.getCompileTaskName(platform.getLanguage().toString()), JavaCompile.class)
+            .configure(task -> {


Suggested change

.named(outputSourceSet.getCompileTaskName(platform.getLanguage().toString()), JavaCompile.class)

.configure(task -> {

.named(outputSourceSet.getCompileTaskName(platform.getLanguage().toString()), JavaCompile.class, task -> {

* Disable Jacoco for platform tests (#37) * 0.0.46 release (previous 0.0.45) + release notes updated [ci skip] * Fixed presto UDF patch broken link (#41) Co-authored-by: Sushant Raikar <[email protected]> * FileSystemUtils: remove an unreliable check for unit testing (#42) Why: we don't need it What changed: instead of checking configuration for hints on whether we are unit testing, we trust the URI's protocol Tests performed: ./gradlew build * 0.0.47 release (previous 0.0.46) + release notes updated [ci skip] * Presto: Pass custom configuration object when using FileSystemUtils (#43) * 0.0.48 release (previous 0.0.47) + release notes updated [ci skip] * Presto: Make ScalarFunctionImplementation state independent of StdUdfWrapper (#44) * 0.0.49 release (previous 0.0.48) + release notes updated [ci skip] * Upgrade to PrestoSQL 333 (#45) Some major changes: - `SqlFunction`, `SqlScalarFunction` and `ScalarFunctionImplementation` have evolved in trinodb/trino#1764 - `Metadata::getScalarFunctionImplementation` evolved in trinodb/trino#1039 - Type signature parser was moved to presto-main in trinodb/trino#1738 * 0.0.50 release (previous 0.0.49) + release notes updated [ci skip] * Add support for StdFloat, StdDouble, and StdBinary (#46) * Introduce StdFloat, StdDouble, and StdBinary interfaces * Add implementations of those interfaces in Avro, Hive, Presto, Spark, and Generic type systems * Add examples of transport UDFs on those new types, and add tests for those UDFs * Update documentation * 0.0.51 release (previous 0.0.50) + release notes updated [ci skip] * Allow users to override main and test source set names, output directories * 0.0.52 release (previous 0.0.51) + release notes updated [ci skip] * Empty commit to release new version * Empty commit to release new version [ci skip-compare-publications] * 0.0.53 release (previous 0.0.52) + release notes updated [ci skip] * Plugin: Publish Presto thin jar which allows consumers to control dependency graph (#49) * 0.0.54 release (previous 0.0.53) + release notes updated [ci skip] * Hive: Struct data should not be converted to object array during StdStruct creation (#50) * Remove slf4j-log4j12 from Transport dependency graph (#51) * Bump shipkit (#54) * 0.0.55 release (previous 0.0.54) + release notes updated [ci skip] * Fix test SQL generation for binary inputs (#55) * 0.0.56 release (previous 0.0.55) + release notes updated [ci skip] * Spark: Create index-based iterator for non-mutable map keySet and values access (#58) * 0.0.57 release (previous 0.0.56) + release notes updated [ci skip] * Avro: Support simple union schemas (#60) * 0.0.58 release (previous 0.0.57) + release notes updated [ci skip] * Support conversion of String type to Utf8 in AvroWrapper (#61) * 0.0.59 release (previous 0.0.58) + release notes updated [ci skip] * Add Avro ENUM read support and fix String bug (#62) Co-authored-by: Raymond Lam <[email protected]> * 0.0.60 release (previous 0.0.59) + release notes updated [ci skip] * Build: Fail if there are checkstyle violations (#64) - Change the Checkstyle severity level from warning to error - Eliminate all existing checkstyle violations Co-authored-by: Carl Steinbach <[email protected]> * 0.0.61 release (previous 0.0.60) + release notes updated [ci skip] * Add travis-build.sh for pre-commit testing from command line (#65) * Add travis-build.sh for pre-commit testing from command line * Fix name of build file in comment Co-authored-by: Carl Steinbach <[email protected]> Co-authored-by: Shardul Mahadik <[email protected]> * Upgrade to Gradle 6.7 (#67) * Support builds with platform specific JDK (#69) * Bump Avro dependency to 1.10.2 (from 1.7.7). (#71) There doesn't seem to be any impact to the code. gradle build passes. * Migrate from PrestoSQL to Trino (#68) * Automate artifact publication to Maven Central (#72) * Update ci.yml java version to 8 (#77) skip release * Fix org.pentaho:pentaho-aggdesigner-algorithm sunset problem (#78) * Remove travis build in favor of github actions (#87) * Add scala_2.11 and scala_2.12 support (#85) * Update ci.yml to also build the udf-examples folder (#90) Co-authored-by: Malini Mahalakshmi Venkatachari <[email protected]> * Fix running multiple builds in run step in workflow action (#92) Co-authored-by: Malini Mahalakshmi Venkatachari <[email protected]> * A solution to fix running multiple UDFs in Spark issue (#93) Co-authored-by: Kai Xu <[email protected]> Co-authored-by: Shardul Mahadik <[email protected]> Co-authored-by: shipkit-org <[email protected]> Co-authored-by: Sushant Raikar <[email protected]> Co-authored-by: Sushant Raikar <[email protected]> Co-authored-by: Suren Nihalani <[email protected]> Co-authored-by: Xingyuan Lin <[email protected]> Co-authored-by: Khai Tran <[email protected]> Co-authored-by: John Joyce <[email protected]> Co-authored-by: Raymond <[email protected]> Co-authored-by: curtiscwang <[email protected]> Co-authored-by: Raymond Lam <[email protected]> Co-authored-by: Carl Steinbach <[email protected]> Co-authored-by: Carl Steinbach <[email protected]> Co-authored-by: Akshay Rai <[email protected]> Co-authored-by: Sreeram Ramachandran <[email protected]> Co-authored-by: Raymond Zhang <[email protected]> Co-authored-by: KAI XU <[email protected]> Co-authored-by: Sushant Raikar <[email protected]> Co-authored-by: Malini Mahalakshmi Venkatachari <[email protected]> Co-authored-by: Kai Xu <[email protected]>

Isolate engines and allow configuration of different jdks on the client

9bd4224

shardulm94 reviewed Apr 20, 2021

View reviewed changes

Addressed comments from Shardul

dfc4722

akshayrai changed the title ~~Isolate engines and allow configuration of different jdks on the client~~ Build plugin with platform specific JDK Apr 24, 2021

fixed checkstyles

ff32bfa

shardulm94 reviewed Apr 24, 2021

View reviewed changes

akshayrai added 2 commits April 25, 2021 18:33

address comments - cleaned up references

0719536

checkin missing files

a4904a9

shardulm94 approved these changes Apr 26, 2021

View reviewed changes

shardulm94 merged commit dd6e2a7 into linkedin:master Apr 26, 2021

akshayrai deleted the jdk branch April 26, 2021 02:16

akshayrai mentioned this pull request Apr 26, 2021

Migrate from PrestoSQL to Trino #68

Merged

rzhang10 mentioned this pull request May 13, 2021

Update ci.yml java version to 8 #77

Merged

wmoustafa pushed a commit that referenced this pull request Oct 13, 2021

Support builds with platform specific JDK (#69)

8709e97

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build plugin with platform specific JDK #69

Build plugin with platform specific JDK #69

akshayrai commented Apr 19, 2021 •

edited

Loading

shardulm94 Apr 20, 2021

akshayrai Apr 21, 2021

shardulm94 Apr 20, 2021

akshayrai Apr 21, 2021

akshayrai commented Apr 24, 2021

shardulm94 left a comment

shardulm94 Apr 24, 2021

akshayrai Apr 26, 2021

shardulm94 Apr 24, 2021

akshayrai Apr 26, 2021

shardulm94 Apr 24, 2021

akshayrai Apr 26, 2021

		.named(outputSourceSet.getCompileTaskName(platform.getLanguage().toString()), JavaCompile.class)
		.configure(task -> {

Build plugin with platform specific JDK #69

Build plugin with platform specific JDK #69

Conversation

akshayrai commented Apr 19, 2021 • 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

akshayrai commented Apr 24, 2021

shardulm94 left a 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

akshayrai commented Apr 19, 2021 •

edited

Loading