Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ZEPPELIN-6143] Add Interpreter Event Server Port configuration option #4893

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

tbonelee
Copy link
Contributor

@tbonelee tbonelee commented Nov 3, 2024

What is this PR for?

This PR introduces the ZEPPELIN_EVENT_SERVER_PORT configuration option.
When this option is set, the Event server that communicates with interpreters will listen on the specified port.

The ZEPPELIN_EVENT_SERVER_PORT option takes precedence over ZEPPELIN_SERVER_RPC_PORTRANGE option.
If ZEPPELIN_EVENT_SERVER_PORT is set, the ZEPPELIN_SERVER_RPC_PORTRANGE range will be ignored.

This option is particularly useful when running individual Zeppelin components in container orchestration environments like Kubernetes.
By explicitly specifying the port, it allows for a more declarative and clearer configuration of service resources compared to ZEPPELIN_SERVER_RPC_PORTRANGE.

What type of PR is it?

Improvement

Todos

  • - Task

What is the Jira issue?

How should this be tested?

  • Run Zeppelin with and without this configuration and check zeppelin-server logs for the message "InterpreterEventServer is starting at ". Verify that the specified port is used as intended(If the option is set).

Questions:

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

@Reamer
Copy link
Contributor

Reamer commented Nov 5, 2024

I understand your approach. Currently I use the ZEPPELIN_SERVER_RPC_PORTRANGE 12320:12320 in my K8s landscape to configure a K8s-Service cleanly.
Why did you decide on the name ZEPPELIN_EVENT_SERVER_PORT, wouldn't ZEPPELIN_SERVER_RPC_PORT be better?

@tbonelee
Copy link
Contributor Author

tbonelee commented Nov 9, 2024

@Reamer
I considered both ZEPPELIN_EVENT_SERVER_PORT and ZEPPELIN_SERVER_RPC_PORT. I chose the former because the term "SERVER" in the latter could be too general, making it unclear which server it refers to, and ZEPPELIN_SERVER_RPC_PORT may be easily confused with ZEPPELIN_SERVER_RPC_PORTRANGE due to the similarity in names.

However, aligning with previous variable names might improve predictability as well, which is why I didn't have a strong preference for the former option from the beginning. What do you think?

@Reamer
Copy link
Contributor

Reamer commented Nov 11, 2024

I favor ZEPPELIN_SERVER_RPC_PORT. Can you remove portRange as a class variable of RemoteInterpreterEventServer and perform the evaluation of the port or portrange in the start()-method.
Furthermore, I do not see any documentation for ZEPPELIN_SERVER_RPC_PORTRANGE you can also add it near the documentation for ZEPPELIN_SERVER_RPC_PORT .

@tbonelee
Copy link
Contributor Author

@Reamer Thanks for the feedback. I've added some commits based on your suggestions:

  • Renamed the ZEPPELIN_EVENT_SERVER_PORT variable to ZEPPELIN_SERVER_RPC_PORT.
  • Removed portRange variable from RemoteInterpreterEventServer.
  • Added documentation for the ZEPPELIN_SERVER_RPC_PORTRANGE and ZEPPELIN_INTERPRETER_RPC_PORTRANGE variables.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants