[ZEPPELIN-5999] Reduce instance objects from Zeppelin #4726
Conversation
Reamer
changed the title
Reamer
force-pushed
the
conf_mini
branch
2 times, most recently
from
a8ddf85
to
6fe6219
Compare
|
I know that the PR is very big. Does anyone feel able to do a review here? |
| @@ -33,7 +35,7 @@ | |||
| */ | |||
| public interface NotebookRepo extends Closeable { | |||
|
|
|||
| void init(ZeppelinConfiguration zConf) throws IOException; | |||
| void init(ZeppelinConfiguration zConf, Gson gson) throws IOException; | |||
There was a problem hiding this comment.
I think this is a public API, and users may have custom implementations, such change will break them. Additionally, we'd better avoid leaking the third-party lib classes as much as possible.
There was a problem hiding this comment.
Very good advice. I have wrapped the parser NoteParser in its own interface. It should now be possible to implement a custom parser easily. The implementation of the interface must of course be done by the people who don't want the default (GsonNoteParser).
GSON will not become more widespread with the change and we can also switch more easily via this interface.
| @@ -30,7 +30,6 @@ public class InterpreterOption { | |||
| public static final transient String SHARED = "shared"; | |||
| public static final transient String SCOPED = "scoped"; | |||
| public static final transient String ISOLATED = "isolated"; | |||
| private static ZeppelinConfiguration conf = ZeppelinConfiguration.create(); | |||
There was a problem hiding this comment.
how about converting this static field from static to a member field? then we don't need to change the signature of getOwners method.
some thoughts: current change makes the API inconsistent, only part of getXXX methods require conf parameter
There was a problem hiding this comment.
I don't really like the implementation. See ec3c312
What do you think?
| return conf; | ||
| } | ||
| return ZeppelinConfiguration.create(null); | ||
| public static ZeppelinConfiguration load() { |
There was a problem hiding this comment.
I see the impl change, but what's the benefit of renaming method create to load
There was a problem hiding this comment.
With this name change, it was easier to see the usage. Also, I think create as a method name is not really well chosen, it loads the configuration from a file. I already have a reload method in mind, but that is far in the future.
Reamer
force-pushed
the
conf_mini
branch
2 times, most recently
from
64df684
to
dbce76b
Compare
Reamer
requested review from
pan3793 and
jongyoul
and removed request for
pan3793
Reamer
force-pushed
the
conf_mini
branch
2 times, most recently
from
2f2ec2d
to
4a11e24
Compare
|
Ready for review. |
|
Thank you for your review. My plan is to merge #4746 first and then do this MR. |
|
LGTM. Please feel free to merge it when you're available. |
What is this PR for?
This PR removes the most important instance objects from Zeppelin.
This PR standardizes our integration tests via the
ZeppelinServerMiniclass.I was previously unaware that the Zeppelin configuration is also used within interpreters. With these changes I noticed it and the Zeppelin configuration is now available to interpreters.
I have tried to split the PR into meaningful commits. Unfortunately, it is not possible to split the PR further due to the mass of uses of the instance objects.
The
ZeppelinServerMiniclass makes it easy to run integration tests in the IDE. Separate steps such as "download Spark", "download Hadoop", "add SPARK_HOME env" or "start Zeppelin" become obsolete.You can now also see quite clearly which tests are actually simple unit tests and which should actually be executed as integration tests. A switch to integration tests can be made in a separate pull request.
My IDE and locale are set to German. Java adopts this and also outputs some exceptions in German. I don't know how we deal with this, so I have removed language specific asserts.
What type of PR is it?
Todos
What is the Jira issue?
How should this be tested?
Questions: