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-5962] LivyInterpreter support more parameter when create session #4657

Merged
merged 2 commits into from
Sep 18, 2023

Conversation

zhaomoran
Copy link
Contributor

@zhaomoran zhaomoran commented Sep 12, 2023

What is this PR for?

LivyInterpreter support more parameter when create session

What type of PR is it?

Improvement

Todos

  • - Task

What is the Jira issue?

How should this be tested?

  • Strongly recommended: add automated unit tests for any new or changed behavior
  • Outline any manual steps to test the PR here.

Screenshots (if appropriate)

Questions:

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

}

public String toJson() {
return gson.toJson(this);
JsonObject jsonObject = new JsonObject();
Copy link
Member

Choose a reason for hiding this comment

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

You might miss import statement for JsonObject?

@jongyoul
Copy link
Member

@zhaomoran Hello, thank you for your contribution. I left a comment and could you please add test cases? I read your ticket and looks fine but I think it would be better to have a test for this case.

@zhaomoran
Copy link
Contributor Author

@jongyoul OK,I just presented the idea here. If the community thinks it's a good idea, I'll add test cases to it.

@jongyoul
Copy link
Member

I believe that your contribution must help others. :-)

@zhaomoran zhaomoran requested a review from jongyoul September 15, 2023 08:49
@Reamer
Copy link
Contributor

Reamer commented Sep 15, 2023

Please perform a rebase. I have recently migrated the tests to JUnit5. Currently there are merge conflicts.

@zhaomoran
Copy link
Contributor Author

@jongyoul This is my first time submitting a PR. Is there anything else I need to do?

@jongyoul
Copy link
Member

@zhaomoran The last step is making all CI green. We have some flaky tests but I usually try to execute some failed tests again to make it green. Your contribution itself is proper to be merged. Could you please wait for a while until the CI becomes green?

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

@jongyoul jongyoul merged commit 5de2d03 into apache:master Sep 18, 2023
31 checks passed
@jongyoul
Copy link
Member

@zhaomoran Thank you for your first contribution. I hope you contribute more and more. I merged your PR and assigned you with the JIRA ticket as well.

akoira pushed a commit to akoira/zeppelin that referenced this pull request Feb 1, 2024
…ssion (apache#4657)

* LivyInterpreter support more parameter when create session

* Add test for ZEPPELIN-5962

---------

Co-authored-by: mrzhao <mrzhao@iflytek.com>
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.

3 participants