You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by doanduyhai <gi...@git.apache.org> on 2016/08/29 14:19:07 UTC

[GitHub] zeppelin pull request #1369: [ZEPPELIN-1376] Add proxy credentials for depen...

GitHub user doanduyhai reopened a pull request:

    https://github.com/apache/zeppelin/pull/1369

    [ZEPPELIN-1376] Add proxy credentials for dependency repo for corporate firewall use-cases

    ### What is this PR for?
    When using Zeppelin behind corporate firewall, sometimes the dependencies download just fails silently. This PR has 2 objectives:
    
    * add proxy credentials information for dependencies repo
    * raise clear error message in case of dependencies download failure
    
    There are 3 commits.
    
    The first one add extra inputs in the form for adding new repository
    
    ![add_repo](https://cloud.githubusercontent.com/assets/1532977/18017489/0b486fda-6bd2-11e6-90c7-ceda18c53575.png)
    
    The second commit fixes some issues and display a clear and explicit error message when download of dependencies fail.
    
    Before that, when the download fails, we can see the below behaviour
    
    ![irrelevant_double_error_message](https://cloud.githubusercontent.com/assets/1532977/18017541/3cf0de1e-6bd2-11e6-8285-af03f222e8d2.gif)
    
    * the error message is displayed twice because the call twice the method `checkDownloadingDependencies();`. One in the success callback of:
    
    ```javascript
     $scope.updateInterpreterSetting = function(form, settingId) {
                  ...
                $http.put(baseUrlSrv.getRestApiBase() + '/interpreter/setting/' + settingId, request)
                  .success(function(data, status, headers, config) {
                    $scope.interpreterSettings[index] = data.body;
                    removeTMPSettings(index);
                    thisConfirm.close();
                    checkDownloadingDependencies();
                    $route.reload();
                  })
                  .error(function(data, status, headers, config) {
                 ...
        };
    ```
    
    Another call is inside success callback of `getInterpreterSettings()`
    
    ```javascript
    var getInterpreterSettings = function() {
          $http.get(baseUrlSrv.getRestApiBase() + '/interpreter/setting')
          .success(function(data, status, headers, config) {
            $scope.interpreterSettings = data.body;
            checkDownloadingDependencies();
          }).error(function(data, status, headers, config) {
          ....
    ```
    
    The problem is that `$route.reload();` in the success callback of `updateInterpreterSetting()` will trigger `init()` then `getInterpreterSettings()` so `checkDownloadingDependencies()` is called twice.
    
    I remove the call to `checkDownloadingDependencies()` from success callback of `updateInterpreterSetting()` 
    
    The second modification is on class `DependencyResolver`. In the screen capture above, we get a **cryptic** NullPointerException coming from `DefaultRepositorySystem`. I now catch this NPE to wrap it into a more sensible and clearer exception:
    
    ```java
    
      public List<ArtifactResult> getArtifactsWithDep(String dependency,
        Collection<String> excludes) throws RepositoryException {
        Artifact artifact = new DefaultArtifact(dependency);
        DependencyFilter classpathFilter = DependencyFilterUtils.classpathFilter(JavaScopes.COMPILE);
        PatternExclusionsDependencyFilter exclusionFilter =
                new PatternExclusionsDependencyFilter(excludes);
    
        CollectRequest collectRequest = new CollectRequest();
        collectRequest.setRoot(new Dependency(artifact, JavaScopes.COMPILE));
    
        synchronized (repos) {
          for (RemoteRepository repo : repos) {
            collectRequest.addRepository(repo);
          }
        }
        DependencyRequest dependencyRequest = new DependencyRequest(collectRequest,
                DependencyFilterUtils.andFilter(exclusionFilter, classpathFilter));
        
     //Catch NPE thrown by aether and give a proper error message 
        try {
          return system.resolveDependencies(session, dependencyRequest).getArtifactResults();
        } catch (NullPointerException ex) {
          throw new RepositoryException(String.format("Cannot fetch dependencies for %s", dependency));
        }
      }
    ```
    
    The result is much more cleaner
    
    ![dependencies_download_error_popup](https://cloud.githubusercontent.com/assets/1532977/18033855/1be5fe9a-6d2e-11e6-91f9-2f5ea66cab26.gif)
    
    
    
    The last commit is just doc update
    
    ![updated_docs](https://cloud.githubusercontent.com/assets/1532977/18017797/97302f14-6bd3-11e6-97cc-77bd52f25cde.png)
    
    
    ### What type of PR is it?
    [Improvement]
    
    ### Todos
    * [ ] - Code Review
    * [ ] - Simple test with no Internet connection
    * [ ] - Test within a corporate firewall env with a third-party dependency, requiring download
    
    ### What is the Jira issue?
    **[ZEPPELIN-1376]**
    
    ### How should this be tested?
    
    ##### Simple test
    * `git fetch origin pull/1369/head:WebProxy`
    * `git checkout WebProxy`
    * `mvn clean package -DskipTests`
    * `bin/zeppelin-daemon.sh restart`
    * disconnect from the Internet (pull out the cable, shutdown wifi ...)
    * add a random dependency to the Spark interpreter (take `info.archinnov:achilles-core:4.2.2` for example)
    * validate the change, you should see an error popup on the top-right corner saying that Zeppelin cannot download the dependency
    
    ##### Corporate firewall test
    * follow the steps above for simple test
    * create a new repository (see how to **[here]**) and set the proxy information
    * retry the steps above to ensure that the download is successful
    
     
    ### Screenshots (if appropriate)
    See above
    
    ### Questions:
    * Does the licenses files need update? --> **NO**
    * Is there breaking changes for older versions? --> **NO**
    * Does this needs documentation?  --> **YES, DONE**
    
    [ZEPPELIN-1376]: https://issues.apache.org/jira/browse/ZEPPELIN-1376
    [here]: http://localhost:4000/manual/dependencymanagement.html

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/doanduyhai/incubator-zeppelin ZEPPELIN-1376

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/zeppelin/pull/1369.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #1369
    
----
commit e33b3db43844ea8dca8d52caacee948cddc0a27a
Author: DuyHai DOAN <do...@gmail.com>
Date:   2016-08-26T14:49:33Z

    [ZEPPELIN-1376] Add proxy credentials information for dependencies repo

commit 084f2bf098c01b609476eaa7a7be67cec1048e79
Author: DuyHai DOAN <do...@gmail.com>
Date:   2016-08-26T18:48:06Z

    [ZEPPELIN-1376] Raise clear error message in case of dependencies download failure

commit 70e350c72e3f9e0273df372102c77c51a9c6e155
Author: DuyHai DOAN <do...@gmail.com>
Date:   2016-08-26T19:05:04Z

    [ZEPPELIN-1376] Update documentation

commit 134f86fe1b2d6e86dc1b676bf7b3c996aa8a696d
Author: DuyHai DOAN <do...@gmail.com>
Date:   2016-08-28T12:42:41Z

    Add unit test and fix impl for DependencyResolver to catch NPE

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---