Skip to content

Conversation

@cloverhearts
Copy link
Member

What is this PR for?

Restful api - call upon the setting / interpreter id type,
The problem occurs in the status of running Paragraphs.

  • Paragraphs status code changes and restart missing

What type of PR is it?

Bug Fix

Todos

Is there a relevant Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-544

How should this be tested?

Paragraphs running after interpreter config change, save -> click okay on restart dialog

Screenshots (if appropriate)

before

bug_fix_before

after

bug_fix_after

Questions:

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

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to refactor setPropertyAndRestart() and restart() to remove duplication and prevent diverged issue like this.

Copy link
Member Author

Choose a reason for hiding this comment

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

@HeartSaVioR Okay, I will attempt to code refactoring.

@HeartSaVioR
Copy link
Contributor

Great for finding missed spot!

@cloverhearts
Copy link
Member Author

@HeartSaVioR Thank you for Feedback. have a nice day!

@HeartSaVioR
Copy link
Contributor

Minor: How about extracting whole synchronized block to new private method?

Actually setPropertyAndRestart() and restart() takes same logic except setting additional parameter and calling saveToFile().

setPropertyAndRestart() can call saveToFile() safely when new method is executed without InterpreterException. So new private method doesn't need to take care about safeToFile().

Next concern is that how we can unify logic whether two parameters are presented or not.
It would be achieved easily with null check, but someone can think it's not beautiful.
So I'd like to hear our opinions about this suggestion.

@cloverhearts
Copy link
Member Author

@HeartSaVioR
I agree with your opinion.
But this is a PR for a bug-fix.
I do not want to put undue changes here.
I am concerned that a new bug occurs.
Once I quit the modification here.

@HeartSaVioR
Copy link
Contributor

@cloverhearts OK, Don't worry. I'll address it to another issue. :) Happy new year!

@cloverhearts
Copy link
Member Author

@HeartSaVioR Happy new year, HeartSaVioR!

@HeartSaVioR
Copy link
Contributor

Forgot to say LGTM. :)

@Leemoonsoo
Copy link
Member

LGTM and merge if there're no more discussions

@asfgit asfgit closed this in 6744c0d Jan 3, 2016
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