Skip to content

[ZEPPELIN-6188] Improve Elasticsearch interpreter config and help message #4929

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

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

Conversation

ParkGyeongTae
Copy link
Contributor

@ParkGyeongTae ParkGyeongTae commented May 4, 2025

What is this PR for?

The following improvements have been made to the Elasticsearch interpreter:

  • Updated default interpreter settings for better clarity and consistency.
    • Ensured accurate property types and added missing environment variable mappings.
    • Improved descriptions for better user understanding in the Zeppelin UI.
    • Changed default port to 9200 and client type to http to align with Elasticsearch 8.x, where the Transport Client has been removed.
  • Modified the help command output to improve readability and accuracy.
    • Reformatted command usage examples.
    • Fixed grammar and clarified parameter descriptions.

These changes enhance the usability of the interpreter and make it easier for users to configure and understand how to interact with Elasticsearch through Zeppelin.

What type of PR is it?

Refactoring

Todos

  • - Update /elasticsearch/src/main/resources/interpreter-setting.json file
  • - Update /elasticsearch/src/main/java/org/apache/zeppelin/elasticsearch/ElasticsearchInterpreter.java file

What is the Jira issue?

How should this be tested?

  • help output command
%elasticsearch
help
  • interpreter -> elasticsearch -> check description

Screenshots (if appropriate)

Questions:

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

@Reamer
Copy link
Contributor

Reamer commented May 6, 2025

With the change of the default port to 9200 and the client type there is a breaking change in my opinion.
What do you think?

@ParkGyeongTae
Copy link
Contributor Author

@Reamer
Yes, I agree — this is indeed a breaking change.

The reason for changing the default port to 9200 and the client type to http is that Elasticsearch officially removed the Transport Client in version 8.0. It had already been deprecated since 7.0 and was no longer recommended for use.

Since the transport client is no longer available, it makes sense to update the default settings accordingly. While this change may affect users relying on the legacy transport protocol, it's a necessary step to ensure compatibility with modern versions of Elasticsearch.

We should document this clearly in the release notes and, if possible, provide migration guidance for users still on older versions.

@Reamer
Copy link
Contributor

Reamer commented May 6, 2025

Can you please adjust the pull request description.

@ParkGyeongTae
Copy link
Contributor Author

@Reamer
I have added Changed default port to 9200 and client type to http to align with Elasticsearch 8.x, where the Transport Client has been removed. to the pull request description, and also updated the corresponding Jira issue.

Copy link
Contributor

@Reamer Reamer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@Reamer Reamer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ParkGyeongTae
Checkstyle fails, please adjust the line.

[INFO] There is 1 error reported by Checkstyle 9.3 with zeppelin/checkstyle.xml ruleset.
Error:  src/main/java/org/apache/zeppelin/elasticsearch/ElasticsearchInterpreter.java:[88] (sizes) LineLength: Line is longer than 100 characters (found 103).
[INFO] ------------------------------------------------------------------------

@ParkGyeongTae
Copy link
Contributor Author

@Reamer
Thank you for the feedback. I've addressed the Checkstyle violation by adjusting the line—I'd appreciate it if you could review it again.

Copy link
Contributor

@Reamer Reamer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, let's wait for CI

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