You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by GitBox <gi...@apache.org> on 2021/02/21 14:55:38 UTC

[GitHub] [zeppelin] zjffdu opened a new pull request #4058: [ZEPPELIN-5257] Refactoring of ExecutionContext

zjffdu opened a new pull request #4058:
URL: https://github.com/apache/zeppelin/pull/4058


   ### 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
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zeppelin] asfgit closed pull request #4058: [ZEPPELIN-5257] Refactoring of ExecutionContext

Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #4058:
URL: https://github.com/apache/zeppelin/pull/4058


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zeppelin] Reamer commented on pull request #4058: [ZEPPELIN-5257] Refactoring of ExecutionContext

Posted by GitBox <gi...@apache.org>.
Reamer commented on pull request #4058:
URL: https://github.com/apache/zeppelin/pull/4058#issuecomment-783221407


   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".
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zeppelin] zjffdu commented on pull request #4058: [ZEPPELIN-5257] Refactoring of ExecutionContext

Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4058:
URL: https://github.com/apache/zeppelin/pull/4058#issuecomment-799806672


   Will merge if no more comment


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zeppelin] zjffdu commented on pull request #4058: [ZEPPELIN-5257] Refactoring of ExecutionContext

Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4058:
URL: https://github.com/apache/zeppelin/pull/4058#issuecomment-783412736


   @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`. 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org