Skip to content

[ZEPPELIN-5257] Refactoring of ExecutionContext#4058

Closed
zjffdu wants to merge 1 commit intoapache:masterfrom
zjffdu:ZEPPELIN-5257
Closed

[ZEPPELIN-5257] Refactoring of ExecutionContext#4058
zjffdu wants to merge 1 commit intoapache:masterfrom
zjffdu:ZEPPELIN-5257

Conversation

@zjffdu
Copy link
Contributor

@zjffdu zjffdu commented Feb 21, 2021

What is this PR for?

This PR is to refactoring ExecutionContext: move its creation to Note, because most of fields of ExecutionContext is from Note. Moving it to Note (Note#getExecutionContext) can help us to create consistent object (avoid missing fields)

What type of PR is it?

[Refactoring]

Todos

  • - Task

What is the Jira issue?

How should this be tested?

  • CI pass

Screenshots (if appropriate)

Questions:

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

@zjffdu zjffdu force-pushed the ZEPPELIN-5257 branch 2 times, most recently from c46af55 to 5535e2d Compare February 22, 2021 03:22
@Reamer
Copy link
Contributor

Reamer commented Feb 22, 2021

In general, I like the builder pattern for ExecutionContext and also final as a keyword for attributes in ExecutionContext.
I also think that binding the ExecutionContext to a Note is a good idea.

I see that you are changing attributes of the ExecutionContext such as the user. I think it would be better to create an additional method in the ExecutionContextBuilder that takes another ExecutionContext and makes a "copy".

@zjffdu
Copy link
Contributor Author

zjffdu commented Feb 22, 2021

@Reamer The reason of moving ExecutionContext to Note is that Note is the only place to create ExecutionContext, and it makes sense because ExecutionContext should always be associated with a Note. So I don't think it is necessary to introduce ExecutionContextBuilder.

@zjffdu
Copy link
Contributor Author

zjffdu commented Mar 15, 2021

Will merge if no more comment

@asfgit asfgit closed this in 85ed8e2 Mar 18, 2021
asfgit pushed a commit that referenced this pull request Mar 18, 2021
### What is this PR for?

This PR is to refactoring ExecutionContext: move its creation to `Note`, because most of fields of ExecutionContext is from Note. Moving it to `Note` (Note#getExecutionContext) can help us to create consistent object (avoid missing fields)

### What type of PR is it?
[Refactoring]

### Todos
* [ ] - Task

### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-5257

### How should this be tested?
* CI pass

### Screenshots (if appropriate)

### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No

Author: Jeff Zhang <zjffdu@apache.org>

Closes #4058 from zjffdu/ZEPPELIN-5257 and squashes the following commits:

84768c1 [Jeff Zhang] [ZEPPELIN-5257] Refactoring of ExecutionContext

(cherry picked from commit 85ed8e2)
Signed-off-by: Jeff Zhang <zjffdu@apache.org>
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

Comments