[ZEPPELIN-6355] Merge zeppelin-zengine to zeppelin-server

[jongyoul]

What is this PR for?

This PR merges the zeppelin-zengine module into zeppelin-server to simplify the project architecture. The zengine module contained core engine functionality for notebook management, paragraph execution, and interpreter management, which is tightly coupled with the server. This merge reduces build complexity while preserving all git history using git mv for file movements.

What type of PR is it?

Refactoring

Todos

• Move all source files from zeppelin-zengine to zeppelin-server using git mv (183 files)
• Merge zengine dependencies into zeppelin-server pom.xml
• Update zeppelin-plugins to depend on zeppelin-server instead of zeppelin-zengine
• Update zeppelin-interpreter-integration dependencies
• Update zeppelin-integration dependencies
• Remove zeppelin-zengine module from root pom.xml
• Update GitHub Actions workflows
• Remove zeppelin-zengine directory
• Verify full build succeeds: ./mvnw clean package

What is the Jira issue?

• ZEPPELIN-6355

How should this be tested?

• CI

Screenshots (if appropriate)

Questions:

• Does the license files need to update? No
• Is there breaking changes for older versions? No
• Does this needs documentation? No

[jongyoul]

@Reamer could you please check it? We might resolve this issue easier than we think.

[tbonelee]

I looked into the failing Selenium test to find the cause.

My working hypothesis is that maven-shade-plugin is rewriting a string-literal, the default value of ZEPPELIN_CONFIG_STORAGE_CLASS inside zeppelin-interpreter's ZeppelinConfiguration. I didn't directly inspect the effective classpath, but the behavior suggests that when the shaded class is resolved first, the rewritten default appears, and when the original class is resolved first the unmodified default remains. This inference comes from the two checks below.

1. Shade raw-string check : Adding <rawString>true</rawString> in the relocation option makes the ZEPPELIN_CONFIG_STORAGE_CLASS default value to remain as the original literal value.
2. Classpath order check : Reordering dependencies so that zeppelin-server appears before spark-interpreter also gives the same result as 1. Before the zengine and server merge, the zeppelin-zengine dependency also appeared before spark-interpreter.

Sharing these observations in case they help fix the failing Selenium test.

zeppelin/zeppelin-interpreter/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java

Line 1037 in 190ecd6

"org.apache.zeppelin.storage.LocalConfigStorage"),

[tbonelee]

There are still some references to zeppelin-zengine left in the shell scripts under /bin.
(e.g., https://github.com/jongyoul/zeppelin/blob/5cc2196bf7afc172cf6f9f7d7cc5f64f9f70b31b/bin/interpreter.sh#L115)

[Reamer]

Thank you very much for the initiative.

We should also include the following exclusion.

diff --git a/zeppelin-server/pom.xml b/zeppelin-server/pom.xml
index ffd14eb1b..37f4b1bb4 100644
--- a/zeppelin-server/pom.xml
+++ b/zeppelin-server/pom.xml
@@ -114,6 +114,11 @@
           <groupId>commons-httpclient</groupId>
           <artifactId>commons-httpclient</artifactId>
         </exclusion>
+        <!-- using jcl-over-slf4j instead -->
+        <exclusion>
+          <groupId>commons-logging</groupId>
+          <artifactId>commons-logging</artifactId>
+        </exclusion>
       </exclusions>
     </dependency>
 
@@ -195,6 +200,11 @@
           <groupId>org.apache.hadoop</groupId>
           <artifactId>hadoop-hdfs-client</artifactId>
         </exclusion>
+        <!-- using jcl-over-slf4j instead -->
+        <exclusion>
+          <groupId>commons-logging</groupId>
+          <artifactId>commons-logging</artifactId>
+        </exclusion>
       </exclusions>
     </dependency>
 

[Reamer]

commons-logging should be excluded, it's done via jcl-over-slf4j

[jongyoul]

Thank you for pointing out. I'll handle it soon. Moreover, we have an issue related to logging library and I will solve it as well.

[tbonelee]

How about switching to the unshaded ZeppelinConfiguration as in this commit? (2. from my previous comment.) Maybe rawString fixes some cases, but by skipping required shaded classes it creates new issues.
It seems to fix all the failing jobs, though I'd prefere a more explicit fix if possible.
(Failed E2E test was fixed in other PR)

[Reamer]

@tbonelee
Have you found any good documentation regarding rawString?

[tbonelee]

On the official site, the only reference I could find is the Javadoc link for the type used by the <relocations> property (https://maven.apache.org/components/plugins/maven-shade-plugin/apidocs/org/apache/maven/plugins/shade/relocation/SimpleRelocator.html), which doesn’t provide much detail.

I skimmed the code and initially assumed rawString was an option to exclude string literals in .java files from being shaded (https://github.com/apache/maven-shade-plugin/blob/073e7ca970c15a8cae64e2de9517fe8d471b5373/src/main/java/org/apache/maven/plugins/shade/relocation/SimpleRelocator.java#L217). I only tried the rawString option to quickly check whether our test failures were caused by shading string literals in java code, so I didn’t investigate it more rigorously.

However, looking at the issue where this option was introduced (https://issues.apache.org/jira/projects/MSHADE/issues/MSHADE-104?filter=allissues) and re-read the whole code of SimpleRelocator, the primary intent seems to be treating patterns as regex. If that’s the case, a pattern like com.google would be interpreted as a regex, which could lead to unexpected relocations.

I’ve struck through my earlier explanation about rawString since it was inaccurate. Sorry about that.

[tbonelee]

Quick question with limited context. Why is ZeppelinConfiguration in the zeppelin-interpreter module? It looks like a general configuration class, so I wondered why it lives under interpreter. Because it sits in zeppelin-interpreter, it gets shaded and tends to be used first, which might relate to the failing tests. If it’s reasonable, would moving ZeppelinConfiguration to a more general module be a better fit and also resolve the tests?

[Reamer]

Why is ZeppelinConfiguration in the zeppelin-interpreter module?

In my opinion, we should split this up. ZeppelinConfiguration belongs to the Zeppelin server, and the Zeppelin interpreter should really only work on a HashMap with ConfigKey and ConfigValue.

The entire loading of the configuration and possibly also a reload should all belong to the server.

Currently, the Zeppelin server transmits the configuration from the server to the remote interpreter when it starts up. See

zeppelin/zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/remote/RemoteInterpreterServer.java

Lines 202 to 207 in a03dcbc

	public void init(Map<String, String> properties) throws InterpreterRPCException, TException {
	this.zConf = ZeppelinConfiguration.load();
	for (Map.Entry<String, String> entry : properties.entrySet()) {
	this.zConf.setProperty(entry.getKey(), entry.getValue());
	}

[seung-00]

@jongyoul
Hi, It seems this PR has been inactive for some time. If you’re okay with it, I’d be happy to take over and continue the work.

[jongyoul]

@seung-00 Thank you! It's also a blocker for your work, and I'd be happy for you to help solve it.