Remove kyuubi dependency of the spark lineage plugin #3537

wForget · 2022-09-21T09:29:31Z

Why are the changes needed?

Remove kyuubi dependency of the spark lineage plugin.

How was this patch tested?

Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before make a pull request

wForget · 2022-09-21T12:15:42Z

cc @pan3793 @iodone

codecov-commenter · 2022-09-21T13:09:47Z

Codecov Report

Merging #3537 (1cd2f54) into master (66f28ef) will increase coverage by 0.92%.
The diff coverage is 48.71%.

@@             Coverage Diff              @@
##             master    #3537      +/-   ##
============================================
+ Coverage     51.66%   52.58%   +0.92%     
  Complexity       13       13              
============================================
  Files           482      494      +12     
  Lines         26933    27737     +804     
  Branches       3760     3834      +74     
============================================
+ Hits          13914    14585     +671     
- Misses        11663    11758      +95     
- Partials       1356     1394      +38

Impacted Files	Coverage Δ
...bi/plugin/lineage/helper/SparkListenerHelper.scala	`33.33% <ø> (ø)`
...in/lineage/helper/SparkSQLLineageParseHelper.scala	`62.70% <0.00%> (ø)`
...kyuubi/plugin/lineage/helper/SemanticVersion.scala	`39.28% <39.28%> (ø)`
.../SparkOperationLineageQueryExecutionListener.scala	`66.66% <50.00%> (ø)`
.../plugin/lineage/events/OperationLineageEvent.scala	`66.66% <100.00%> (+2.66%)`	⬆️
...ache/spark/kyuubi/lineage/SparkContextHelper.scala	`100.00% <100.00%> (ø)`
...apache/kyuubi/service/TBinaryFrontendService.scala	`47.77% <0.00%> (-39.98%)`	⬇️
...rg/apache/kyuubi/ctl/cmd/log/LogBatchCommand.scala	`63.93% <0.00%> (-14.07%)`	⬇️
...kyuubi/engine/spark/session/SparkSessionImpl.scala	`61.53% <0.00%> (-9.05%)`	⬇️
...che/kyuubi/server/KyuubiTHttpFrontendService.scala	`59.86% <0.00%> (-4.37%)`	⬇️
... and 82 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

iodone · 2022-09-22T09:39:35Z

cc @pan3793 @iodone

What is the reason for the removal? Is it possible to have the lineage plugin applied to the custom spark engine that is not Kyubi Engine?

wForget · 2022-09-22T09:59:46Z

Is it possible to have the lineage plugin applied to the custom spark engine that is not Kyubi Engine?

Yes, I would prefer it to remove kyuubi dependencies and be able to use it in any spark application.

ulysses-you · 2022-09-22T10:06:38Z

is it possible to shade the kyuubi dependencies into lineage plugin ?

yikf · 2022-09-22T15:41:07Z

is it possible to shade the kyuubi dependencies into lineage plugin ?

Just a quesion, what's the reason for wanting shade? to reduce code redundancy?

I think the shade approach would have redundant multiple jars in the Kyuubi scenario, and given the background of not much redundant code in the current approach, I prefer to remove the dependencies directly, what do you think?

iodone · 2022-09-23T02:11:54Z

...bi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/SparkSQLEngineEventListener.scala

+      case e: SparkListenerEvent if SparkUtilsHelper.lineageClassesArePresent =>
+        e match {
+          case OperationLineageEventWrapper(lineageEvent) =>
+            updateStatementLineageStore(lineageEvent)


Here we can collect both operationEvent and lineageEvent, whether to consider adding statementId into the operationEvent with the same executionId

Here we can collect both operationEvent and lineageEvent, whether to consider adding statementId into the operationEvent with the same executionId

If we want to get the statementId we may need to use SQLOperationListener and make larger change, should we do that?

How about we get the statementId via qe.sparkSession.sparkContext.getLocalProperty("kyuubi.statement.id") in SparkOperationLineageQueryExecutionListener#onSuccess?

SparkOperationEvent was been stored in the same kvstore with LineageEvent , can we associate them here?

The listener is a separate thread from the execution thread of the spark session, and I'm not sure if we can get it correctly.

The KVStore seems to require statementId to get SparkOperationEvent.

You are right, it seems to be difficult to correlate the two.

iodone · 2022-09-23T02:14:42Z

...park-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/events/EngineEventsStore.scala

@@ -61,6 +61,14 @@ class EngineEventsStore(store: KVStore) {
    }
  }

+  def getStatementLineage(executionId: String): Option[SparkOperationLineageEvent] = {
+    try {


This method has never been called?

This method has never been called?

Yes, it's just for convenience we use it later such as displaying lineage in the Spark UI. do i need to remove it?

wForget · 2022-09-26T01:43:57Z

@iodone , thanks for your review, sorry for replying so late.

iodone · 2022-09-28T01:55:13Z

@wForget #3444, In order to implement the PlanOnly mode of lineage, imported SQLParserHelper with spark-lineage-plugin. This will cause the kyuubi spark engine to strongly depend on spark-lineage-plugin.
Are you in favor of this usage, and if that holds true, your implementation here could just refer directly to spark-lineage-plugin. WDYT?

wForget · 2022-09-28T02:24:03Z

@wForget #3444, In order to implement the PlanOnly mode of lineage, imported SQLParserHelper with spark-lineage-plugin. This will cause the kyuubi spark engine to strongly depend on spark-lineage-plugin. Are you in favor of this usage, and if that holds true, your implementation here could just refer directly to spark-lineage-plugin. WDYT?

I don't tend to add lineage plugin dependencies in kyuubi engine, because in the future we may make more adaptations in lineage plugins, such as we can send lineage events to kafla, atlas, etc. Maybe we can extract a lineage common module to make kyuubi engine depend on.

iodone · 2022-09-28T12:45:52Z

externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/SparkUtilsHelper.scala

+  def lineageClassesArePresent: Boolean = {
+    try {
+      Utils.classForName(
+        "org.apache.kyuubi.plugin.lineage.SparkOperationLineageQueryExecutionListener")


@yaooqinn #3444 Can we refer to this method, kyuubi-spark-engine depend on the lineage plugin using provided scope

No, I'm not talking about the maven scope. kyuubi-spark-engine should not depend on an engine plugin。

yaooqinn · 2022-09-29T09:42:12Z

A general question, why does a spark plugin have to rely on the kyuubi code path?

wForget · 2022-09-29T10:28:32Z

A general question, why does a spark plugin have to rely on the kyuubi code path?

The current lineage plugin is more like a kyuubi spark engine plugin, we rely on kyuubi-events module to produce kyuubi lineage event. This pr wants to change lineage to spark event, in order to remove kyuubi dependency.
However, I thought we might use lineage spark event in engine later such as show it on spark ui, so I introduced lineage plugin dependency in kyuubi engine again.

yaooqinn · 2022-09-29T10:32:11Z

So we can not call it a plugin, it's a upstream package

yaooqinn · 2022-09-29T10:33:41Z

oh, I mess this PR with @iodone's one

yaooqinn · 2022-09-29T10:35:40Z

...i-spark-lineage/src/main/scala/org/apache/kyuubi/plugin/lineage/helper/SemanticVersion.scala

+ * Encapsulate a component (Kyuubi/Spark/Hive/Flink etc.) version
+ * for the convenience of version checks.
+ */
+case class SemanticVersion(majorVersion: Int, minorVersion: Int) {


hmm, before introducing this, the version comparison seems to be very simple. Now, plenty of copies of this class.

hmm, before introducing this, the version comparison seems to be very simple. Now, plenty of copies of this class.

In this plugin we only use isSparkVersionAtMost method, do I need to simply reimplement a method instead of copying SemanticVersion?

pan3793 · 2022-11-01T13:36:35Z

Glancing at the comments, I think the author and reviewers reached a consensus the lineage plugin should be a pure Spark plugin and should not depend on kyuubi-common, kyuubi-event modules, but have some arguments on the relationship between kyuubi-spark-sql-engine and kyuubi-spark-lineage, so, can we narrow this PR scope into making kyuubi-spark-lineage independent w/ kyuubi-event?

Some users want to use this plugin along w/ the latest release kyuubi-spark-sql-engine but got class conflict error since there were incompatible changes that occurred in kyuubi-event.

wForget · 2022-11-02T01:11:33Z

Glancing at the comments, I think the author and reviewers reached a consensus the lineage plugin should be a pure Spark plugin and should not depend on kyuubi-common, kyuubi-event modules, but have some arguments on the relationship between kyuubi-spark-sql-engine and kyuubi-spark-lineage, so, can we narrow this PR scope into making kyuubi-spark-lineage independent w/ kyuubi-event?

Good idea. I will narrow this PR scope.

wForget · 2022-11-02T07:25:48Z

seems unrelated failure:

AdminResourceSuite:
- delete engine - user share level *** FAILED ***
  List("serviceUri=localhost:32971;version=1.7.0-SNAPSHOT;refId=8bd0f165-37a6-49cf-af67-d82f7a0b3cbb;sequence=0000000000") had size 1 instead of expected size 0 (AdminResourceSuite.scala:103)

iodone · 2022-11-02T08:26:15Z

seems unrelated failure:

AdminResourceSuite:
- delete engine - user share level *** FAILED ***
  List("serviceUri=localhost:32971;version=1.7.0-SNAPSHOT;refId=8bd0f165-37a6-49cf-af67-d82f7a0b3cbb;sequence=0000000000") had size 1 instead of expected size 0 (AdminResourceSuite.scala:103)

I have the same problem in #3558 , but other unrelated changes do not trigger this failure case.

wForget · 2022-11-02T08:45:18Z

seems unrelated failure:
AdminResourceSuite:
- delete engine - user share level *** FAILED ***
  List("serviceUri=localhost:32971;version=1.7.0-SNAPSHOT;refId=8bd0f165-37a6-49cf-af67-d82f7a0b3cbb;sequence=0000000000") had size 1 instead of expected size 0 (AdminResourceSuite.scala:103)
I have the same problem in #3558 , but other unrelated changes do not trigger this failure case.

This test case doesn't use the kyuubi-spark-lineage plugin, let me re-run the check.

iodone · 2022-11-03T06:13:21Z

seems unrelated failure:
AdminResourceSuite:
- delete engine - user share level *** FAILED ***
  List("serviceUri=localhost:32971;version=1.7.0-SNAPSHOT;refId=8bd0f165-37a6-49cf-af67-d82f7a0b3cbb;sequence=0000000000") had size 1 instead of expected size 0 (AdminResourceSuite.scala:103)
I have the same problem in #3558 , but other unrelated changes do not trigger this failure case.
This test case doesn't use the kyuubi-spark-lineage plugin, let me re-run the check.

In my case, this failure case still exists with re-running

wForget · 2022-11-03T10:52:05Z

In my case, this failure case still exists with re-running

It is being followed up in #3753, let's try again later.

pan3793 · 2022-11-03T15:46:30Z

Thanks, merging to master

wForget added 3 commits September 21, 2022 14:11

Remove kyuubi dependency of the spark lineage plugin

9e44840

fix test

d746579

fix and add test

71b1150

github-actions bot added kind:build module:extensions module:spark labels Sep 21, 2022

iodone reviewed Sep 23, 2022

View reviewed changes

iodone mentioned this pull request Sep 28, 2022

[KYUUBI #3444] Support the planOnly mode of kyuubi spark engine support SQL lineage #3558

Closed

3 tasks

iodone reviewed Sep 28, 2022

View reviewed changes

yaooqinn reviewed Sep 29, 2022

View reviewed changes

comment

1cd2f54

wForget force-pushed the dev-lineage branch from 0614286 to 1cd2f54 Compare November 2, 2022 01:48

pan3793 approved these changes Nov 3, 2022

View reviewed changes

pan3793 assigned wForget Nov 3, 2022

pan3793 added this to the v1.7.0 milestone Nov 3, 2022

pan3793 closed this in 7f83c45 Nov 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove kyuubi dependency of the spark lineage plugin #3537

Remove kyuubi dependency of the spark lineage plugin #3537

wForget commented Sep 21, 2022

wForget commented Sep 21, 2022

codecov-commenter commented Sep 21, 2022 •

edited

Loading

iodone commented Sep 22, 2022

wForget commented Sep 22, 2022

ulysses-you commented Sep 22, 2022

yikf commented Sep 22, 2022 •

edited

Loading

iodone Sep 23, 2022

wForget Sep 26, 2022

wForget Sep 26, 2022

iodone Sep 26, 2022

wForget Sep 26, 2022

iodone Sep 26, 2022

iodone Sep 23, 2022

wForget Sep 26, 2022

wForget commented Sep 26, 2022

iodone commented Sep 28, 2022

wForget commented Sep 28, 2022

iodone Sep 28, 2022

yaooqinn Sep 29, 2022

yaooqinn commented Sep 29, 2022 •

edited

Loading

wForget commented Sep 29, 2022

yaooqinn commented Sep 29, 2022

yaooqinn commented Sep 29, 2022

yaooqinn Sep 29, 2022 •

edited

Loading

wForget Nov 2, 2022

pan3793 commented Nov 1, 2022 •

edited

Loading

wForget commented Nov 2, 2022

wForget commented Nov 2, 2022

iodone commented Nov 2, 2022

wForget commented Nov 2, 2022

iodone commented Nov 3, 2022

wForget commented Nov 3, 2022

pan3793 commented Nov 3, 2022

Remove kyuubi dependency of the spark lineage plugin #3537

Remove kyuubi dependency of the spark lineage plugin #3537

Conversation

wForget commented Sep 21, 2022

Why are the changes needed?

How was this patch tested?

wForget commented Sep 21, 2022

codecov-commenter commented Sep 21, 2022 • edited Loading

Codecov Report

iodone commented Sep 22, 2022

wForget commented Sep 22, 2022

ulysses-you commented Sep 22, 2022

yikf commented Sep 22, 2022 • 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

wForget commented Sep 26, 2022

iodone commented Sep 28, 2022

wForget commented Sep 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaooqinn commented Sep 29, 2022 • edited Loading

wForget commented Sep 29, 2022

yaooqinn commented Sep 29, 2022

yaooqinn commented Sep 29, 2022

yaooqinn Sep 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pan3793 commented Nov 1, 2022 • edited Loading

wForget commented Nov 2, 2022

wForget commented Nov 2, 2022

iodone commented Nov 2, 2022

wForget commented Nov 2, 2022

iodone commented Nov 3, 2022

wForget commented Nov 3, 2022

pan3793 commented Nov 3, 2022

codecov-commenter commented Sep 21, 2022 •

edited

Loading

yikf commented Sep 22, 2022 •

edited

Loading

yaooqinn commented Sep 29, 2022 •

edited

Loading

yaooqinn Sep 29, 2022 •

edited

Loading

pan3793 commented Nov 1, 2022 •

edited

Loading