[ZEPPELIN-6085] Add configuration option for setting the default UI #4824

tbonelee · 2024-09-11T15:55:36Z

What is this PR for?

This PR introduces a new configuration option zeppelin.default.ui to set the default UI for Zeppelin.
The available options for this setting are new and classic.
By default, the value is set to new, meaning the new UI will be served at the root path ("/"), while the classic UI will be available at "/classic".
If the configuration is set to classic, the classic UI will be served at "/", and the new UI at "/new".

What type of PR is it?

Improvement

Todos

- Task

What is the Jira issue?

Open an issue on Jira https://issues.apache.org/jira/browse/ZEPPELIN/6085
Put link here, and add [ZEPPELIN-Jira number] in PR title, eg. [ZEPPELIN-533]

How should this be tested?

Testing the new option :
- Set the zeppelin.default.ui config to new (the default).
- Verify that the new UI is served at the root ("/") and the classic UI is available at "/classic".
- Verify that navigating between pages works without any issue.
- Verify that the UI switching button works properly in both UIs.
Testing classic option :
- Similar steps as the above, except the new UI is served at "/classic" and the classic UI is available at the root ("/")

Screenshots (if appropriate)

Questions:

Does the license files need to update? No
Is there breaking changes for older versions? No
Does this needs documentation? Yes, documentation for the new configuration option has been added.

Reamer

I have made some comments on the Java class. I don't know if the changes are possible in the Java script frontends. Aren't the URLs to other elements (images, CSS etc.) fixed during the building of the frontends?

zeppelin-server/src/main/java/org/apache/zeppelin/server/ZeppelinServer.java

tbonelee · 2024-09-13T11:53:50Z

@Reamer
It seems like it could depend on the situation.
In my opinion, if the resource URLs are set as fixed absolute paths, there could be an issue, but if they are relative paths, there shouldn't be a problem.

In the initial code I wrote, I added code on the server side to insert a base tag into the index.html at runtime.
Howver, when I ran the app and checked the network tab, resources were being fetched from the correct paths even without the base tag, so I removed it.

From reviewing the frontend code, my assumption is that most of the resources are set with relative paths, or they dynamically generate absolute paths based on each app's path, so there didn't seem to be an issue.

However, upon reconsideration, I thought explicitly specifying the base tag would be a more reliable soltuion, so I added the code back in.

tbonelee · 2024-09-13T12:00:26Z

Additionally, I missed this part earlier, but for the MathJax library, the library root path for resolving the module loading URL needs to be changed.
However, it was set to always fetch from the root regardless of the app's path. (In fact, this caused the new UI app, which was served from the /next path, to fetch the MathJax code from the classic UI app's resource path.)

So, I made additional modifications to ensure this part changes according to the app's path.

jongyoul · 2024-09-17T02:10:51Z

It includes various types of code :-) Thank you for your work. I'll test it sooner than later.

tbonelee · 2024-09-22T13:50:54Z

@Reamer @jongyoul Just pinging to review :)

…operly at runtime

tbonelee · 2024-09-29T11:31:15Z

I've rebased this branch and added documentation for configuring the default Ui on the explore_ui page.

zeppelin-server/src/main/java/org/apache/zeppelin/server/ZeppelinServer.java

…linServer.java

zeppelin-server/src/main/java/org/apache/zeppelin/server/ZeppelinServer.java

…linServer.java

Reamer · 2024-10-01T11:06:04Z

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

@@ -905,6 +910,10 @@ public boolean isPrometheusMetricEnabled() {
    return getBoolean(ConfVars.ZEPPELIN_METRIC_ENABLE_PROMETHEUS);
  }

+  public DEFAULT_UI getDefaultUi() {
+    return DEFAULT_UI.valueOf(getString(ConfVars.ZEPPELIN_DEFAULT_UI).toUpperCase());


What happens if I pass neither new nor classic as a configuration value at this point?

If neither new nor classic is provided as a configuration value, the valueOf() method will throw an IllegalArgumentException during server startup.
I based this behavior on the getRunMode() method, which similarly doesn't handle exceptions for invalid input.

Should we add more graceful error handling here?

Does it mean that we set the configuration as an empty value? It would be proper to throw an exception for the case.

@jongyoul If the value is empty, getString() will return default value (new) and if an invalid value (neither new nor classic) is provided, it will throw an exception.

Do you think it is not appropriate to provide a default value for an empty value in this configuration?

In my opinion, the exception and the termination of the start in the event of incorrect configuration is acceptable.

Reamer · 2024-10-01T11:18:32Z

zeppelin-web-angular/src/index.html

@@ -44,6 +43,14 @@ <h1 class="brand-title" style="display: none">Zeppelin</h1>
        "HTML-CSS": { availableFonts: ["TeX"] },
        messageStyle: "none"
      }
+      const origin = window.location.origin;


Why is this needed?

MathJax resolves its dependency URLs based on its configuration.
Previously, it was loading its dependency files from the root URL (the default UI app URL), regardless of whether it was being served from the default UI or the /next UI.
Since both apps included MathJax as a dependency, this didn't cause any issues, but I felt it was more appropriate for MathJax to load its dependencies from the correct app-specific paths.

So, I updated th MathJax configurations to ensure it resolves URLs relative to the correct app.

Reamer · 2024-10-01T11:23:49Z

zeppelin-web-angular/src/index.html

@@ -17,7 +17,6 @@
  <meta name="viewport" content="width=device-width, initial-scale=1">
  <link rel="icon" href="./assets/images/zeppelin.png" type="image/x-icon">
  <title>Zeppelin</title>
-  <base href="/">


You take this out in the new UI, which I understand. Where is the place in the old UI?

@Reamer Both the old and new UIs are served from their respective sub-paths (depending on the configuration, either /, /new, or /classic), and as I understand it, the trailing part of the URL after #/... is treated as an anchor and ignored.
Therefore, without the base tag, static resources should be correctly fetched from the appropriate web app serving path, since the src attributes for those resources are set using relative paths.
I verified this in the browser, and everything worked as expected.

Given this, I believe neither app requires a base tag. In fact the old UI didn't have one to begin with.

I understand your question as asking where the base tag is in the old UI, so I've responded accordingly.
However, if I misunderstood your point, please feel free to clarify.

Reamer

I looked at the code and it might work. Tests would of course be nice, but are difficult to implement at such a high level to test this small feature.
I have not tested manually myself and take your word for it.

### What is this PR for? This PR introduces a new configuration option `zeppelin.default.ui` to set the default UI for Zeppelin. The available options for this setting are `new` and `classic`. By default, the value is set to `new`, meaning the new UI will be served at the root path (`"/"`), while the classic UI will be available at `"/classic"`. If the configuration is set to `classic`, the classic UI will be served at `"/"`, and the new UI at `"/new"`. ### What type of PR is it? Improvement ### Todos * [ ] - Task ### What is the Jira issue? * Open an issue on Jira https://issues.apache.org/jira/browse/ZEPPELIN/6085 * Put link here, and add [ZEPPELIN-*Jira number*] in PR title, eg. [ZEPPELIN-533] ### How should this be tested? - Testing the `new` option : - Set the `zeppelin.default.ui` config to `new` (the default). - Verify that the new UI is served at the root (`"/"`) and the classic UI is available at `"/classic"`. - Verify that navigating between pages works without any issue. - Verify that the UI switching button works properly in both UIs. - Testing `classic` option : - Similar steps as the above, except the new UI is served at `"/classic"` and the classic UI is available at the root (`"/"`) ### Screenshots (if appropriate) ### Questions: * Does the license files need to update? No * Is there breaking changes for older versions? No * Does this needs documentation? Yes, documentation for the new configuration option has been added. Closes #4824 from tbonelee/add-default-ui-config. Signed-off-by: Jongyoul Lee <[email protected]> (cherry picked from commit 00b7c55) Signed-off-by: Jongyoul Lee <[email protected]>

Reamer requested changes Sep 12, 2024

View reviewed changes

zeppelin-server/src/main/java/org/apache/zeppelin/server/ZeppelinServer.java Outdated Show resolved Hide resolved

zeppelin-server/src/main/java/org/apache/zeppelin/server/ZeppelinServer.java Outdated Show resolved Hide resolved

tbonelee added 13 commits September 29, 2024 19:20

Add zeppelin.default.ui config. (new,classic)

fff6475

Use default ui config to set web app context at runtime

22d7598

Rename variables

f71e8ba

Remove base href build option for new UI app

868b084

Remove base tag in new UI to make the app resolve static file URLs pr…

4b15f61

…operly at runtime

Set href for UI switch btn in new UI in runtime

8e848e1

Set href for UI switch btn in classic UI in runtime

2b10b30

Add a row in the configuration document

1ea213d

Add configuration in zeppelin-site.xml.template

819a73c

Fix MathJax dynamic module load path in zeppelin-web

fcca610

Fix MathJax dynamic module load path in zeppelin-web-angular

3d5d907

Manipulate base tags in index.html

0f394e3

Reflecting PR reviews

bd701ac

tbonelee force-pushed the add-default-ui-config branch from 86e158c to bd701ac Compare September 29, 2024 10:20

Add documentation in explore_ui

eb31aac

pan3793 reviewed Sep 29, 2024

View reviewed changes

zeppelin-server/src/main/java/org/apache/zeppelin/server/ZeppelinServer.java Outdated Show resolved Hide resolved

Update zeppelin-server/src/main/java/org/apache/zeppelin/server/Zeppe…

024d845

…linServer.java

pan3793 reviewed Sep 29, 2024

View reviewed changes

zeppelin-server/src/main/java/org/apache/zeppelin/server/ZeppelinServer.java Outdated Show resolved Hide resolved

Update zeppelin-server/src/main/java/org/apache/zeppelin/server/Zeppe…

e84e0c4

…linServer.java

pan3793 requested a review from Reamer September 29, 2024 14:20

jongyoul approved these changes Oct 1, 2024

View reviewed changes

Reamer reviewed Oct 1, 2024

View reviewed changes

Reamer approved these changes Oct 8, 2024

View reviewed changes

jongyoul merged commit 00b7c55 into apache:master Oct 9, 2024
28 checks passed

tbonelee deleted the add-default-ui-config branch October 9, 2024 06:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ZEPPELIN-6085] Add configuration option for setting the default UI #4824

[ZEPPELIN-6085] Add configuration option for setting the default UI #4824

tbonelee commented Sep 11, 2024

Reamer left a comment

tbonelee commented Sep 13, 2024

tbonelee commented Sep 13, 2024

jongyoul commented Sep 17, 2024

tbonelee commented Sep 22, 2024 •

edited

Loading

tbonelee commented Sep 29, 2024

Reamer Oct 1, 2024

tbonelee Oct 1, 2024

jongyoul Oct 1, 2024

tbonelee Oct 1, 2024

Reamer Oct 8, 2024

Reamer Oct 1, 2024

tbonelee Oct 1, 2024

Reamer Oct 1, 2024

tbonelee Oct 1, 2024 •

edited

Loading

Reamer left a comment

[ZEPPELIN-6085] Add configuration option for setting the default UI #4824

[ZEPPELIN-6085] Add configuration option for setting the default UI #4824

Conversation

tbonelee commented Sep 11, 2024

What is this PR for?

What type of PR is it?

Todos

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

Reamer left a comment

Choose a reason for hiding this comment

tbonelee commented Sep 13, 2024

tbonelee commented Sep 13, 2024

jongyoul commented Sep 17, 2024

tbonelee commented Sep 22, 2024 • edited Loading

tbonelee commented Sep 29, 2024

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

tbonelee Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Reamer left a comment

Choose a reason for hiding this comment

tbonelee commented Sep 22, 2024 •

edited

Loading

tbonelee Oct 1, 2024 •

edited

Loading