Details

    • Type: Sub-task
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.8.0, 3.0.0-alpha1
    • Component/s: ui
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      This sub-task JIRA is for improving the NN HTML5 UI to allow the user to create directories. It uses WebHDFS and adds to the great work done in HDFS-6252

      1. HDFS-7713.01.patch
        117 kB
        Ravi Prakash
      2. HDFS-7713.02.patch
        109 kB
        Ravi Prakash
      3. HDFS-7713.03.patch
        11 kB
        Ravi Prakash
      4. HDFS-7713.04.patch
        11 kB
        Ravi Prakash
      5. HDFS-7713.05.patch
        3 kB
        Ravi Prakash
      6. HDFS-7713.06.patch
        4 kB
        Ravi Prakash
      7. HDFS-7713.07.patch
        4 kB
        Ravi Prakash
      8. HDFS-7713.08.patch
        4 kB
        Ravi Prakash

        Activity

        Hide
        raviprak Ravi Prakash added a comment -

        This is a patch which adds the following features:

        1. Create directories
        2. Change owner / group
        3. Change replication

        Big thanks to a LOT of people: Travis Thompson, Nina Stawski, Howard Weingram and Dragana Mijalkovic.

        Show
        raviprak Ravi Prakash added a comment - This is a patch which adds the following features: Create directories Change owner / group Change replication Big thanks to a LOT of people: Travis Thompson , Nina Stawski , Howard Weingram and Dragana Mijalkovic.
        Hide
        raviprak Ravi Prakash added a comment -

        To test out the patch in your dev environment, you may want to set

          <property>
            <name>hadoop.http.staticuser.user</name>
            <value>SOME-USER-NAME-WITH-ACCESS-EG-hdfs</value>
          </property>
        

        Haohui Mai Could you please review this patch considering all the awesome work you did for converting the UI to HTML5 in the first place?

        Show
        raviprak Ravi Prakash added a comment - To test out the patch in your dev environment, you may want to set <property> <name>hadoop.http.staticuser.user</name> <value>SOME-USER-NAME-WITH-ACCESS-EG-hdfs</value> </property> Haohui Mai Could you please review this patch considering all the awesome work you did for converting the UI to HTML5 in the first place?
        Hide
        wheat9 Haohui Mai added a comment -

        Let's separate the patch into multiple patches for clarity and eases of reviews – maybe we can start with mkdirs?

        Show
        wheat9 Haohui Mai added a comment - Let's separate the patch into multiple patches for clarity and eases of reviews – maybe we can start with mkdirs?
        Hide
        raviprak Ravi Prakash added a comment -

        Hi Haohui! Thanks for the suggestion. Here's an updated patch which only adds the mkdir feature. The patch file itself is deceptively large because it include bootstrap. The changes to explorer.html, explorer.js and hadoop.css are fairly limited (and actually I indented some code properly) so the changes seem larger than they really are.

        Show
        raviprak Ravi Prakash added a comment - Hi Haohui! Thanks for the suggestion. Here's an updated patch which only adds the mkdir feature. The patch file itself is deceptively large because it include bootstrap. The changes to explorer.html, explorer.js and hadoop.css are fairly limited (and actually I indented some code properly) so the changes seem larger than they really are.
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12698220/HDFS-7713.02.patch
        against trunk revision fe689d3.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9549//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12698220/HDFS-7713.02.patch against trunk revision fe689d3. -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9549//console This message is automatically generated.
        Hide
        raviprak Ravi Prakash added a comment -

        The patch doesn't apply because its a binary patch (Including icon images)

        Show
        raviprak Ravi Prakash added a comment - The patch doesn't apply because its a binary patch (Including icon images)
        Hide
        aw Allen Wittenauer added a comment -

        I think if the patch is generated with git format-patch, it will work.

        Show
        aw Allen Wittenauer added a comment - I think if the patch is generated with git format-patch, it will work.
        Hide
        wheat9 Haohui Mai added a comment - - edited

        Can you explain why this patch requires bringing in the bootstrap library? The current UI is using bootstrap already. I don't understand why files like `editable*css` are needed in this patch.

        Show
        wheat9 Haohui Mai added a comment - - edited Can you explain why this patch requires bringing in the bootstrap library? The current UI is using bootstrap already. I don't understand why files like `editable*css` are needed in this patch.
        Hide
        raviprak Ravi Prakash added a comment -

        You're right Haohui! Specifically for mkdir , bootstrap-editable wasn't required. I've removed it in this patch.

        Show
        raviprak Ravi Prakash added a comment - You're right Haohui! Specifically for mkdir , bootstrap-editable wasn't required. I've removed it in this patch.
        Hide
        wheat9 Haohui Mai added a comment -

        Looks like the indentation is off in many places. And things like

        -      <div class="row">
        -        <hr />
        -        <div class="col-xs-2"><p>Hadoop, 2014.</p></div>
        +    <div class="row">
        +      <div class="col-12">
        +        <div id="panel"></div>
               </div>
        +    </div>
         
        +    <div class="row">
        +      <hr/>
        +      <div class="col-2"><p>Hadoop, 2015.</p></div>
             </div>
         
        

        Could be separated into different patches. Can you please go through the patch and try to minimize it?

        Show
        wheat9 Haohui Mai added a comment - Looks like the indentation is off in many places. And things like - <div class= "row" > - <hr /> - <div class= "col-xs-2" ><p>Hadoop, 2014.</p></div> + <div class= "row" > + <div class= "col-12" > + <div id= "panel" ></div> </div> + </div> + <div class= "row" > + <hr/> + <div class= "col-2" ><p>Hadoop, 2015.</p></div> </div> Could be separated into different patches. Can you please go through the patch and try to minimize it?
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12698512/HDFS-7713.03.patch
        against trunk revision 58cb9f5.

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. There were no new javadoc warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs.

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9562//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9562//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12698512/HDFS-7713.03.patch against trunk revision 58cb9f5. +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9562//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9562//console This message is automatically generated.
        Hide
        raviprak Ravi Prakash added a comment -

        Thanks for the suggestion Haohui! Here's a patch where I've tried to fix all the indentation I could find and reverted the "Hadoop, 2015" change

        Show
        raviprak Ravi Prakash added a comment - Thanks for the suggestion Haohui! Here's a patch where I've tried to fix all the indentation I could find and reverted the "Hadoop, 2015" change
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12698569/HDFS-7713.04.patch
        against trunk revision 99f6bd4.

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. There were no new javadoc warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The test build failed in hadoop-hdfs-project/hadoop-hdfs

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9566//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9566//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12698569/HDFS-7713.04.patch against trunk revision 99f6bd4. +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The test build failed in hadoop-hdfs-project/hadoop-hdfs Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9566//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9566//console This message is automatically generated.
        Hide
        raviprak Ravi Prakash added a comment -

        The jenkins build doesn't show any "test failures". Hadoop QA is cuckoo

        Show
        raviprak Ravi Prakash added a comment - The jenkins build doesn't show any "test failures". Hadoop QA is cuckoo
        Hide
        raviprak Ravi Prakash added a comment -

        Hi Haohui Mai! Could you please review the latest patch?

        Show
        raviprak Ravi Prakash added a comment - Hi Haohui Mai ! Could you please review the latest patch?
        Hide
        wheat9 Haohui Mai added a comment -

        Sorry for the delay. Will take a look at it today or tomorrow.

        Show
        wheat9 Haohui Mai added a comment - Sorry for the delay. Will take a look at it today or tomorrow.
        Hide
        wheat9 Haohui Mai added a comment -

        Thanks for updating the patch. There are still a lot of unnecessary changes in the patch. For example:

        -          <a href="#" class="dropdown-toggle" data-toggle="dropdown">Utilities <b class="caret"></b></a>
        +          <a href="#" class="dropdown-toggle" data-toggle="dropdown">Utilities <b class="caret caret-white"></b></a>
        
        -    <div class="modal" id="file-info" tabindex="-1" role="dialog" aria-hidden="true">
        -      <div class="modal-dialog">
        -	<div class="modal-content">
        -	  <div class="modal-header"><button type="button" class="close" data-dismiss="modal" aria-hidden="true">&times;</button>
        -	    <h4 class="modal-title" id="file-info-title">File information</h4>
        -	  </div>
        -	  <div class="modal-body" id="file-info-body">
        -	    <a id="file-info-download">Download</a>
        -        <!--<a id="file-info-preview" style="cursor:pointer">Tail the file (last 32K)</a>-->
        -	    <hr />
        -	    <div class="panel panel-success" id="file-info-blockinfo-panel">
        -	      <div class="panel-heading">
        -		Block information -- 
        -		<select class="btn btn-default" id="file-info-blockinfo-list">
        -		</select>
        -	      </div>
        -	      <div class="panel-body" id="file-info-blockinfo-body"></div>
        -	    </div>
        -	    <div class="panel panel-info" id="file-info-tail" style="display:none">
        -	      <div class="panel-heading">File contents</div>
        -	      <div class="panel-body">
        -		<div class="input-group-sm">
        -		<textarea class="form-control" style="height: 150px" id="file-info-preview-body"></textarea>
        -		</div>
        -	      </div>
        -	    </div>
        -	  </div>
        -	  <div class="modal-footer"><button type="button" class="btn btn-success"
        -					    data-dismiss="modal">Close</button></div>
        -	</div>
        +
        +  <div class="modal" id="file-info" tabindex="-1" role="dialog" aria-hidden="true">
        +    <div class="modal-dialog">
        +      <div class="modal-content">
        +        <div class="modal-header">
        +          <button type="button" class="close" data-dismiss="modal" aria-hidden="true">&times;</button>
        +          <h4 class="modal-title" id="file-info-title">File information</h4>
        +        </div>
        +        <div class="modal-body" id="file-info-body">
        +          <a id="file-info-download">Download</a>
        +          <!--<a id="file-info-preview" style="cursor:pointer">Tail the file (last 32K)</a>-->
        +          <hr/>
        +          <div class="panel panel-success" id="file-info-blockinfo-panel">
        +            <div class="panel-heading">
        +              Block information --
        +              <select class="btn btn-default" id="file-info-blockinfo-list">
        +              </select>
        +            </div>
        +            <div class="panel-body" id="file-info-blockinfo-body"></div>
        +          </div>
        +          <div class="panel panel-info" id="file-info-tail" style="display:none">
        +            <div class="panel-heading">File contents</div>
        +            <div class="panel-body">
        +              <div class="input-group-sm">
        +                <textarea class="form-control" style="height: 150px" id="file-info-preview-body"></textarea>
        +              </div>
        +            </div>
        +          </div>
        +        </div>
        +        <div class="modal-footer">
        +          <button type="button" class="btn btn-success" data-dismiss="modal">Close</button>
        +        </div>
        

        ....

        -      <div class="row">
        -        <hr />
        -        <div class="col-xs-2"><p>Hadoop, 2014.</p></div>
        +    <div class="row">
        +      <div class="col-12">
        +        <div id="panel"></div>
               </div>
        +    </div>
         
        +    <div class="row">
        +      <hr />
        +      <div class="col-xs-2"><p>Hadoop, 2014.</p></div>
             </div>
        

        There are more throughout the patch. For example, I also think that the changes to the CSS is unnecessary.

        I'll separate the feedbacks of the functionality in a new comment.

        Show
        wheat9 Haohui Mai added a comment - Thanks for updating the patch. There are still a lot of unnecessary changes in the patch. For example: - <a href= "#" class= "dropdown-toggle" data-toggle= "dropdown" >Utilities <b class= "caret" ></b></a> + <a href= "#" class= "dropdown-toggle" data-toggle= "dropdown" >Utilities <b class= "caret caret-white" ></b></a> - <div class= "modal" id= "file-info" tabindex= "-1" role= "dialog" aria-hidden= " true " > - <div class= "modal-dialog" > - <div class= "modal-content" > - <div class= "modal-header" ><button type= "button" class= "close" data-dismiss= "modal" aria-hidden= " true " >&times;</button> - <h4 class= "modal-title" id= "file-info-title" >File information</h4> - </div> - <div class= "modal-body" id= "file-info-body" > - <a id= "file-info-download" >Download</a> - <!--<a id= "file-info-preview" style= "cursor:pointer" >Tail the file (last 32K)</a>--> - <hr /> - <div class= "panel panel-success" id= "file-info-blockinfo-panel" > - <div class= "panel-heading" > - Block information -- - <select class= "btn btn- default " id= "file-info-blockinfo-list" > - </select> - </div> - <div class= "panel-body" id= "file-info-blockinfo-body" ></div> - </div> - <div class= "panel panel-info" id= "file-info-tail" style= "display:none" > - <div class= "panel-heading" >File contents</div> - <div class= "panel-body" > - <div class= "input-group-sm" > - <textarea class= "form-control" style= "height: 150px" id= "file-info-preview-body" ></textarea> - </div> - </div> - </div> - </div> - <div class= "modal-footer" ><button type= "button" class= "btn btn-success" - data-dismiss= "modal" >Close</button></div> - </div> + + <div class= "modal" id= "file-info" tabindex= "-1" role= "dialog" aria-hidden= " true " > + <div class= "modal-dialog" > + <div class= "modal-content" > + <div class= "modal-header" > + <button type= "button" class= "close" data-dismiss= "modal" aria-hidden= " true " >&times;</button> + <h4 class= "modal-title" id= "file-info-title" >File information</h4> + </div> + <div class= "modal-body" id= "file-info-body" > + <a id= "file-info-download" >Download</a> + <!--<a id= "file-info-preview" style= "cursor:pointer" >Tail the file (last 32K)</a>--> + <hr/> + <div class= "panel panel-success" id= "file-info-blockinfo-panel" > + <div class= "panel-heading" > + Block information -- + <select class= "btn btn- default " id= "file-info-blockinfo-list" > + </select> + </div> + <div class= "panel-body" id= "file-info-blockinfo-body" ></div> + </div> + <div class= "panel panel-info" id= "file-info-tail" style= "display:none" > + <div class= "panel-heading" >File contents</div> + <div class= "panel-body" > + <div class= "input-group-sm" > + <textarea class= "form-control" style= "height: 150px" id= "file-info-preview-body" ></textarea> + </div> + </div> + </div> + </div> + <div class= "modal-footer" > + <button type= "button" class= "btn btn-success" data-dismiss= "modal" >Close</button> + </div> .... - <div class= "row" > - <hr /> - <div class= "col-xs-2" ><p>Hadoop, 2014.</p></div> + <div class= "row" > + <div class= "col-12" > + <div id= "panel" ></div> </div> + </div> + <div class= "row" > + <hr /> + <div class= "col-xs-2" ><p>Hadoop, 2014.</p></div> </div> There are more throughout the patch. For example, I also think that the changes to the CSS is unnecessary. I'll separate the feedbacks of the functionality in a new comment.
        Hide
        wheat9 Haohui Mai added a comment -
        +        <div class="dirCommands" id="dirCommands">
        +          <button type="button" class="btn btn-primary" data-toggle="modal"
        +            data-target="#create-directory">Create Directory
        +          </button>
        +        </div>
        

        It is unnecessary to define the class and the id. It might also make sense to stick with the current look and feel (i.e., using btn-default instead of btn-primary). It might also make sense to replace it with a icon as well.

        -          var path = $(this).attr('inode-path');
        +          var path = $(this).closest('tr').attr('inode-path');
        

        Doesn't seem necessary.

        +          crossDomain: true
        

        As MKDIR is only a metadata operation, there should be no need for allowing the request to cross origin.

        Show
        wheat9 Haohui Mai added a comment - + <div class= "dirCommands" id= "dirCommands" > + <button type= "button" class= "btn btn-primary" data-toggle= "modal" + data-target= "#create-directory" >Create Directory + </button> + </div> It is unnecessary to define the class and the id. It might also make sense to stick with the current look and feel (i.e., using btn-default instead of btn-primary ). It might also make sense to replace it with a icon as well. - var path = $( this ).attr('inode-path'); + var path = $( this ).closest('tr').attr('inode-path'); Doesn't seem necessary. + crossDomain: true As MKDIR is only a metadata operation, there should be no need for allowing the request to cross origin.
        Hide
        raviprak Ravi Prakash added a comment -

        Hi Haohui! Here's a new patch with all your suggested edits.

        Show
        raviprak Ravi Prakash added a comment - Hi Haohui! Here's a new patch with all your suggested edits.
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12700327/HDFS-7713.05.patch
        against trunk revision faaddb6.

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. There were no new javadoc warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs.

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9652//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9652//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12700327/HDFS-7713.05.patch against trunk revision faaddb6. +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9652//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9652//console This message is automatically generated.
        Hide
        raviprak Ravi Prakash added a comment -

        Hi Haohui Mai Could you please review the latest patch?

        Show
        raviprak Ravi Prakash added a comment - Hi Haohui Mai Could you please review the latest patch?
        Hide
        wheat9 Haohui Mai added a comment -

        Thanks for the ping. Will do it today or tomorrow.

        Show
        wheat9 Haohui Mai added a comment - Thanks for the ping. Will do it today or tomorrow.
        Hide
        wheat9 Haohui Mai added a comment -
        +            <button type="button" class="close"
        +              data-dismiss="modal" aria-hidden="true">&times;</button>
        +            <h4 class="modal-title" id="dir-create-title">Create Directory</h4>
        +          </div>
        

        The id dir-create-title are unnecessary, so as things like dir-create-body. Only elements that are accessed by JavaScript need IDs.

        +              <div class="col-sm-10">
        

        It is unnecessary.

        +                  <input type="text" class="form-control" id="new_directory"
        +                    placeholder="New Directory Name">
        

        It has to be a closed tag to meet the XHTML standard.

        +        <div>&nbsp;</div>
        +        <div>
        +          <button type="button" class="btn btn-default" data-toggle="modal"
        +            data-target="#create-directory">Create Directory
        +          </button>
        +        </div>
        

        It breaks the row layout in bootstrap. You can put the button on the same row using:

            <div class="row">
              <div class="col-xs-11">
                <form onsubmit="return false;">
                  <div class="input-group">
                    <input type="text" class="form-control" id="directory"/>
                    <span class="input-group-btn">
                      <button class="btn btn-default" type="button" id="btn-nav-directory">Go!</button>
                    </span>
                  </div>
                </form>
              </div>
              <div class="col-xs-1">
                <button type="button" class="btn btn-default" data-toggle="modal" aria-label="New Directory"
                        data-target="#create-directory"> <span class="glyphicon glyphicon-folder-open"></span>
                </button>
              </div>
            </div>
        
        +  $('#create-directory').on('show.bs.modal', function(event) {
        +    var modal = $(this)
        +    $('#new_directory_pwd').html(current_directory);
        +    $('#create-directory-button').on('click', function () {
        

        The click handler should be registered outside and should use the click() method directly, otherwise you'll end up with registering multiple click handlers and issue multiple webhdfs request.

        It also makes sense to ID the button with consistent naming style, e.g. btn-create-directory

        +      $.ajax(url,
        +        { type: 'PUT'
        +        }
        

        It might make sense to put it in one line.

        Show
        wheat9 Haohui Mai added a comment - + <button type= "button" class= "close" + data-dismiss= "modal" aria-hidden= " true " >&times;</button> + <h4 class= "modal-title" id= "dir-create-title" >Create Directory</h4> + </div> The id dir-create-title are unnecessary, so as things like dir-create-body . Only elements that are accessed by JavaScript need IDs. + <div class= "col-sm-10" > It is unnecessary. + <input type= "text" class= "form-control" id= "new_directory" + placeholder= "New Directory Name" > It has to be a closed tag to meet the XHTML standard. + <div>&nbsp;</div> + <div> + <button type= "button" class= "btn btn- default " data-toggle= "modal" + data-target= "#create-directory" >Create Directory + </button> + </div> It breaks the row layout in bootstrap. You can put the button on the same row using: <div class= "row" > <div class= "col-xs-11" > <form onsubmit= " return false ;" > <div class= "input-group" > <input type= "text" class= "form-control" id= "directory" /> <span class= "input-group-btn" > <button class= "btn btn- default " type= "button" id= "btn-nav-directory" >Go!</button> </span> </div> </form> </div> <div class= "col-xs-1" > <button type= "button" class= "btn btn- default " data-toggle= "modal" aria-label= "New Directory" data-target= "#create-directory" > <span class= "glyphicon glyphicon-folder-open" ></span> </button> </div> </div> + $('#create-directory').on('show.bs.modal', function(event) { + var modal = $( this ) + $('#new_directory_pwd').html(current_directory); + $('#create-directory-button').on('click', function () { The click handler should be registered outside and should use the click() method directly, otherwise you'll end up with registering multiple click handlers and issue multiple webhdfs request. It also makes sense to ID the button with consistent naming style, e.g. btn-create-directory + $.ajax(url, + { type: 'PUT' + } It might make sense to put it in one line.
        Hide
        wheat9 Haohui Mai added a comment -

        ping...

        I think it is pretty close. Ravi Prakash can you please update the patch if you want to get it in 2.7? Thanks.

        Show
        wheat9 Haohui Mai added a comment - ping... I think it is pretty close. Ravi Prakash can you please update the patch if you want to get it in 2.7? Thanks.
        Hide
        raviprak Ravi Prakash added a comment -

        Thanks for your detailed review Haohui! Much appreciated. Sorry I had been OOO last week. Here's a patch which incorporates all your feedback.

        Show
        raviprak Ravi Prakash added a comment - Thanks for your detailed review Haohui! Much appreciated. Sorry I had been OOO last week. Here's a patch which incorporates all your feedback.
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12705125/HDFS-7713.06.patch
        against trunk revision 487374b.

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. There were no new javadoc warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

        org.apache.hadoop.hdfs.util.TestByteArrayManager
        org.apache.hadoop.hdfs.server.namenode.ha.TestPipelinesFailover

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9938//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9938//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12705125/HDFS-7713.06.patch against trunk revision 487374b. +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.util.TestByteArrayManager org.apache.hadoop.hdfs.server.namenode.ha.TestPipelinesFailover Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9938//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9938//console This message is automatically generated.
        Hide
        raviprak Ravi Prakash added a comment -

        These test failures are most likely spurious because I never touched JAVA code in this patch.

        Show
        raviprak Ravi Prakash added a comment - These test failures are most likely spurious because I never touched JAVA code in this patch.
        Hide
        wheat9 Haohui Mai added a comment -

        The patch looks pretty good to me.

        +    var url = '/webhdfs/v1' + append_path(current_directory,
        +      $('#new_directory').val()) + '?op=MKDIRS';
        

        The path need to be encoded when constructing the path. The current patch will fail when creating a directory whose name is asdf#df+1.

        Show
        wheat9 Haohui Mai added a comment - The patch looks pretty good to me. + var url = '/webhdfs/v1' + append_path(current_directory, + $('#new_directory').val()) + '?op=MKDIRS'; The path need to be encoded when constructing the path. The current patch will fail when creating a directory whose name is asdf#df+1 .
        Hide
        raviprak Ravi Prakash added a comment -

        Thanks a lot Haohui! Great catch! Here a patch which encodes the URI

        Show
        raviprak Ravi Prakash added a comment - Thanks a lot Haohui! Great catch! Here a patch which encodes the URI
        Hide
        raviprak Ravi Prakash added a comment -

        I just noticed that browsing that directory leads to a bad request. I'll file a new JIRA for fixing that.

        Show
        raviprak Ravi Prakash added a comment - I just noticed that browsing that directory leads to a bad request. I'll file a new JIRA for fixing that.
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12706095/HDFS-7713.07.patch
        against trunk revision fe5c23b.

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. There were no new javadoc warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

        org.apache.hadoop.tracing.TestTracing

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/10015//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/10015//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12706095/HDFS-7713.07.patch against trunk revision fe5c23b. +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.tracing.TestTracing Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/10015//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/10015//console This message is automatically generated.
        Hide
        raviprak Ravi Prakash added a comment -

        I've patched HDFS-7968. Could you please review and commit that JIRA too?

        Show
        raviprak Ravi Prakash added a comment - I've patched HDFS-7968 . Could you please review and commit that JIRA too?
        Hide
        raviprak Ravi Prakash added a comment -

        HDFS-7968 turned out to be a duplicate of HDFS-7953.

        Here's a patch which correctly encodes the path using encode_path

        Show
        raviprak Ravi Prakash added a comment - HDFS-7968 turned out to be a duplicate of HDFS-7953 . Here's a patch which correctly encodes the path using encode_path
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12706938/HDFS-7713.08.patch
        against trunk revision 6413d34.

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. There were no new javadoc warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

        org.apache.hadoop.hdfs.TestReplication
        org.apache.hadoop.hdfs.server.datanode.TestDnRespectsBlockReportSplitThreshold
        org.apache.hadoop.tracing.TestTracing

        Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/10052//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/10052//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12706938/HDFS-7713.08.patch against trunk revision 6413d34. +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.TestReplication org.apache.hadoop.hdfs.server.datanode.TestDnRespectsBlockReportSplitThreshold org.apache.hadoop.tracing.TestTracing Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/10052//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/10052//console This message is automatically generated.
        Hide
        wheat9 Haohui Mai added a comment -

        +1. I'll commit it shortly.

        Show
        wheat9 Haohui Mai added a comment - +1. I'll commit it shortly.
        Hide
        wheat9 Haohui Mai added a comment -

        I've committed the patch to trunk and branch-2. Thanks Ravi Prakash for the contribution.

        Show
        wheat9 Haohui Mai added a comment - I've committed the patch to trunk and branch-2. Thanks Ravi Prakash for the contribution.
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-trunk-Commit #7423 (See https://builds.apache.org/job/Hadoop-trunk-Commit/7423/)
        HDFS-7713. Implement mkdirs in the HDFS Web UI. Contributed by Ravi Prakash. (wheat9: rev e38ef70fbc60f062992c834b1cca6e9ba4baef6e)

        • hadoop-hdfs-project/hadoop-hdfs/src/main/webapps/hdfs/explorer.html
        • hadoop-hdfs-project/hadoop-hdfs/src/main/webapps/hdfs/explorer.js
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-trunk-Commit #7423 (See https://builds.apache.org/job/Hadoop-trunk-Commit/7423/ ) HDFS-7713 . Implement mkdirs in the HDFS Web UI. Contributed by Ravi Prakash. (wheat9: rev e38ef70fbc60f062992c834b1cca6e9ba4baef6e) hadoop-hdfs-project/hadoop-hdfs/src/main/webapps/hdfs/explorer.html hadoop-hdfs-project/hadoop-hdfs/src/main/webapps/hdfs/explorer.js hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #143 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/143/)
        HDFS-7713. Implement mkdirs in the HDFS Web UI. Contributed by Ravi Prakash. (wheat9: rev e38ef70fbc60f062992c834b1cca6e9ba4baef6e)

        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • hadoop-hdfs-project/hadoop-hdfs/src/main/webapps/hdfs/explorer.js
        • hadoop-hdfs-project/hadoop-hdfs/src/main/webapps/hdfs/explorer.html
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #143 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/143/ ) HDFS-7713 . Implement mkdirs in the HDFS Web UI. Contributed by Ravi Prakash. (wheat9: rev e38ef70fbc60f062992c834b1cca6e9ba4baef6e) hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/main/webapps/hdfs/explorer.js hadoop-hdfs-project/hadoop-hdfs/src/main/webapps/hdfs/explorer.html
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Hadoop-Yarn-trunk #877 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/877/)
        HDFS-7713. Implement mkdirs in the HDFS Web UI. Contributed by Ravi Prakash. (wheat9: rev e38ef70fbc60f062992c834b1cca6e9ba4baef6e)

        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • hadoop-hdfs-project/hadoop-hdfs/src/main/webapps/hdfs/explorer.js
        • hadoop-hdfs-project/hadoop-hdfs/src/main/webapps/hdfs/explorer.html
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-Yarn-trunk #877 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/877/ ) HDFS-7713 . Implement mkdirs in the HDFS Web UI. Contributed by Ravi Prakash. (wheat9: rev e38ef70fbc60f062992c834b1cca6e9ba4baef6e) hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/main/webapps/hdfs/explorer.js hadoop-hdfs-project/hadoop-hdfs/src/main/webapps/hdfs/explorer.html
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Hdfs-trunk #2075 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2075/)
        HDFS-7713. Implement mkdirs in the HDFS Web UI. Contributed by Ravi Prakash. (wheat9: rev e38ef70fbc60f062992c834b1cca6e9ba4baef6e)

        • hadoop-hdfs-project/hadoop-hdfs/src/main/webapps/hdfs/explorer.js
        • hadoop-hdfs-project/hadoop-hdfs/src/main/webapps/hdfs/explorer.html
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk #2075 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2075/ ) HDFS-7713 . Implement mkdirs in the HDFS Web UI. Contributed by Ravi Prakash. (wheat9: rev e38ef70fbc60f062992c834b1cca6e9ba4baef6e) hadoop-hdfs-project/hadoop-hdfs/src/main/webapps/hdfs/explorer.js hadoop-hdfs-project/hadoop-hdfs/src/main/webapps/hdfs/explorer.html hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #134 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/134/)
        HDFS-7713. Implement mkdirs in the HDFS Web UI. Contributed by Ravi Prakash. (wheat9: rev e38ef70fbc60f062992c834b1cca6e9ba4baef6e)

        • hadoop-hdfs-project/hadoop-hdfs/src/main/webapps/hdfs/explorer.js
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • hadoop-hdfs-project/hadoop-hdfs/src/main/webapps/hdfs/explorer.html
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #134 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/134/ ) HDFS-7713 . Implement mkdirs in the HDFS Web UI. Contributed by Ravi Prakash. (wheat9: rev e38ef70fbc60f062992c834b1cca6e9ba4baef6e) hadoop-hdfs-project/hadoop-hdfs/src/main/webapps/hdfs/explorer.js hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/main/webapps/hdfs/explorer.html
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #143 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/143/)
        HDFS-7713. Implement mkdirs in the HDFS Web UI. Contributed by Ravi Prakash. (wheat9: rev e38ef70fbc60f062992c834b1cca6e9ba4baef6e)

        • hadoop-hdfs-project/hadoop-hdfs/src/main/webapps/hdfs/explorer.html
        • hadoop-hdfs-project/hadoop-hdfs/src/main/webapps/hdfs/explorer.js
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #143 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/143/ ) HDFS-7713 . Implement mkdirs in the HDFS Web UI. Contributed by Ravi Prakash. (wheat9: rev e38ef70fbc60f062992c834b1cca6e9ba4baef6e) hadoop-hdfs-project/hadoop-hdfs/src/main/webapps/hdfs/explorer.html hadoop-hdfs-project/hadoop-hdfs/src/main/webapps/hdfs/explorer.js hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Hadoop-Mapreduce-trunk #2093 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2093/)
        HDFS-7713. Implement mkdirs in the HDFS Web UI. Contributed by Ravi Prakash. (wheat9: rev e38ef70fbc60f062992c834b1cca6e9ba4baef6e)

        • hadoop-hdfs-project/hadoop-hdfs/src/main/webapps/hdfs/explorer.html
        • hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
        • hadoop-hdfs-project/hadoop-hdfs/src/main/webapps/hdfs/explorer.js
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-Mapreduce-trunk #2093 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2093/ ) HDFS-7713 . Implement mkdirs in the HDFS Web UI. Contributed by Ravi Prakash. (wheat9: rev e38ef70fbc60f062992c834b1cca6e9ba4baef6e) hadoop-hdfs-project/hadoop-hdfs/src/main/webapps/hdfs/explorer.html hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt hadoop-hdfs-project/hadoop-hdfs/src/main/webapps/hdfs/explorer.js
        Hide
        raviprak Ravi Prakash added a comment -

        Thanks a lot for your detailed and persistent reviews Haohui Mai . I'll follow the same patterns for the other subtasks of HDFS-7588.

        Show
        raviprak Ravi Prakash added a comment - Thanks a lot for your detailed and persistent reviews Haohui Mai . I'll follow the same patterns for the other subtasks of HDFS-7588 .

          People

          • Assignee:
            raviprak Ravi Prakash
            Reporter:
            raviprak Ravi Prakash
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development