You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Anonymous Coward (Code Review)" <ge...@cloudera.org> on 2018/07/06 15:40:08 UTC

[kudu-CR] Read url-enconded hdfs path on restore 

tony@phdata.io has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10880


Change subject: Read url-enconded hdfs path on restore
......................................................................

Read url-enconded hdfs path on restore

Backup locations are url encoded, while KuduRestore was trying to read
the unmodified path.

Change-Id: Ib028b9239c9e4aa71084757eb2dc16a3068de929
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala
3 files changed, 5 insertions(+), 4 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/10880/1
-- 
To view, visit http://gerrit.cloudera.org:8080/10880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib028b9239c9e4aa71084757eb2dc16a3068de929
Gerrit-Change-Number: 10880
Gerrit-PatchSet: 1
Gerrit-Owner: tony@phdata.io

[kudu-CR] Read url-encoded table name on restore

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10880 )

Change subject: Read url-encoded table name on restore
......................................................................


Patch Set 5: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/10880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib028b9239c9e4aa71084757eb2dc16a3068de929
Gerrit-Change-Number: 10880
Gerrit-PatchSet: 5
Gerrit-Owner: Tony Foerster <to...@phdata.io>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tony Foerster <to...@phdata.io>
Gerrit-Comment-Date: Fri, 06 Jul 2018 20:59:36 +0000
Gerrit-HasComments: No

[kudu-CR] Read url-encoded table name on restore

Posted by "Tony Foerster (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/10880

to look at the new patch set (#4).

Change subject: Read url-encoded table name on restore
......................................................................

Read url-encoded table name on restore

Kudu table names are being url-encoded on backup. Read the url-encoded
table names on restore.

Change-Id: Ib028b9239c9e4aa71084757eb2dc16a3068de929
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
3 files changed, 8 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/10880/4
-- 
To view, visit http://gerrit.cloudera.org:8080/10880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib028b9239c9e4aa71084757eb2dc16a3068de929
Gerrit-Change-Number: 10880
Gerrit-PatchSet: 4
Gerrit-Owner: Tony Foerster <to...@phdata.io>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Read url-encoded table name on restore

Posted by "Tony Foerster (Code Review)" <ge...@cloudera.org>.
Tony Foerster has posted comments on this change. ( http://gerrit.cloudera.org:8080/10880 )

Change subject: Read url-encoded table name on restore
......................................................................


Patch Set 6:

The build failure doesn't look to be related to the test (ClientTest.TestFailedDnsResolution)


-- 
To view, visit http://gerrit.cloudera.org:8080/10880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib028b9239c9e4aa71084757eb2dc16a3068de929
Gerrit-Change-Number: 10880
Gerrit-PatchSet: 6
Gerrit-Owner: Tony Foerster <to...@phdata.io>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tony Foerster <to...@phdata.io>
Gerrit-Comment-Date: Mon, 09 Jul 2018 18:06:54 +0000
Gerrit-HasComments: No

[kudu-CR] Read url-encoded table name on restore

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10880 )

Change subject: Read url-encoded table name on restore
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10880/4/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
File java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala:

http://gerrit.cloudera.org:8080/#/c/10880/4/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@45
PS4, Line 45: enconding
Nit: encoding



-- 
To view, visit http://gerrit.cloudera.org:8080/10880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib028b9239c9e4aa71084757eb2dc16a3068de929
Gerrit-Change-Number: 10880
Gerrit-PatchSet: 4
Gerrit-Owner: Tony Foerster <to...@phdata.io>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tony Foerster <to...@phdata.io>
Gerrit-Comment-Date: Fri, 06 Jul 2018 20:22:40 +0000
Gerrit-HasComments: Yes

[kudu-CR] Read url-encoded table name on restore

Posted by "Tony Foerster (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/10880

to look at the new patch set (#5).

Change subject: Read url-encoded table name on restore
......................................................................

Read url-encoded table name on restore

Kudu table names are being url-encoded on backup. Read the url-encoded
table names on restore.

Change-Id: Ib028b9239c9e4aa71084757eb2dc16a3068de929
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
3 files changed, 8 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/10880/5
-- 
To view, visit http://gerrit.cloudera.org:8080/10880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib028b9239c9e4aa71084757eb2dc16a3068de929
Gerrit-Change-Number: 10880
Gerrit-PatchSet: 5
Gerrit-Owner: Tony Foerster <to...@phdata.io>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tony Foerster <to...@phdata.io>

[kudu-CR] Read url-enconded hdfs path on restore

Posted by "Tony Foerster (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/10880

to look at the new patch set (#2).

Change subject: Read url-enconded hdfs path on restore
......................................................................

Read url-enconded hdfs path on restore

Backup locations are url encoded, while KuduRestore was trying to read
the unmodified path.

Change-Id: Ib028b9239c9e4aa71084757eb2dc16a3068de929
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala
4 files changed, 8 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/10880/2
-- 
To view, visit http://gerrit.cloudera.org:8080/10880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib028b9239c9e4aa71084757eb2dc16a3068de929
Gerrit-Change-Number: 10880
Gerrit-PatchSet: 2
Gerrit-Owner: Tony Foerster <to...@phdata.io>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Read url-encoded table name on restore

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change.  ( http://gerrit.cloudera.org:8080/10880 )

Change subject: Read url-encoded table name on restore
......................................................................


Removed reviewer Kudu Jenkins with the following votes:

* Verified-1 by Kudu Jenkins (120)
-- 
To view, visit http://gerrit.cloudera.org:8080/10880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Ib028b9239c9e4aa71084757eb2dc16a3068de929
Gerrit-Change-Number: 10880
Gerrit-PatchSet: 6
Gerrit-Owner: Tony Foerster <to...@phdata.io>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Tony Foerster <to...@phdata.io>

[kudu-CR] Read url-encoded table name on restore

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10880 )

Change subject: Read url-encoded table name on restore
......................................................................


Patch Set 6: Verified+1 Code-Review+2

Yes, the failure is KUDU-1466, a common source of flakiness that has yet to be fixed.


-- 
To view, visit http://gerrit.cloudera.org:8080/10880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib028b9239c9e4aa71084757eb2dc16a3068de929
Gerrit-Change-Number: 10880
Gerrit-PatchSet: 6
Gerrit-Owner: Tony Foerster <to...@phdata.io>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tony Foerster <to...@phdata.io>
Gerrit-Comment-Date: Mon, 09 Jul 2018 18:22:47 +0000
Gerrit-HasComments: No

[kudu-CR] Read url-enconded hdfs path on restore

Posted by "Tony Foerster (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/10880

to look at the new patch set (#3).

Change subject: Read url-enconded hdfs path on restore
......................................................................

Read url-enconded hdfs path on restore

Backup locations are url encoded, while KuduRestore was trying to read
the unmodified path.

Change-Id: Ib028b9239c9e4aa71084757eb2dc16a3068de929
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
3 files changed, 7 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/10880/3
-- 
To view, visit http://gerrit.cloudera.org:8080/10880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib028b9239c9e4aa71084757eb2dc16a3068de929
Gerrit-Change-Number: 10880
Gerrit-PatchSet: 3
Gerrit-Owner: Tony Foerster <to...@phdata.io>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Read url-encoded table name on restore

Posted by "Tony Foerster (Code Review)" <ge...@cloudera.org>.
Tony Foerster has posted comments on this change. ( http://gerrit.cloudera.org:8080/10880 )

Change subject: Read url-encoded table name on restore
......................................................................


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10880/6/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
File java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala:

http://gerrit.cloudera.org:8080/#/c/10880/6/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@59
PS6, Line 59:   test("Simple Backup and Restore Table Name With Special Characters") {
I pulled the impala special character/url encoding test into its own test, the Impala-style table name wasn't actually being created.



-- 
To view, visit http://gerrit.cloudera.org:8080/10880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib028b9239c9e4aa71084757eb2dc16a3068de929
Gerrit-Change-Number: 10880
Gerrit-PatchSet: 6
Gerrit-Owner: Tony Foerster <to...@phdata.io>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tony Foerster <to...@phdata.io>
Gerrit-Comment-Date: Mon, 09 Jul 2018 15:05:39 +0000
Gerrit-HasComments: Yes

[kudu-CR] Read url-encoded table name on restore

Posted by "Tony Foerster (Code Review)" <ge...@cloudera.org>.
Tony Foerster has posted comments on this change. ( http://gerrit.cloudera.org:8080/10880 )

Change subject: Read url-encoded table name on restore
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10880/3/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
File java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala:

http://gerrit.cloudera.org:8080/#/c/10880/3/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@44
PS3, Line 44:   test("Simple Backup and Restore") {
> What's the effect of this override? What different code path is being teste
The override seems unnecessary. I moved the impala table name string into a single test and added a comment



-- 
To view, visit http://gerrit.cloudera.org:8080/10880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib028b9239c9e4aa71084757eb2dc16a3068de929
Gerrit-Change-Number: 10880
Gerrit-PatchSet: 4
Gerrit-Owner: Tony Foerster <to...@phdata.io>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tony Foerster <to...@phdata.io>
Gerrit-Comment-Date: Fri, 06 Jul 2018 18:59:56 +0000
Gerrit-HasComments: Yes

[kudu-CR] Read url-encoded table name on restore

Posted by "Tony Foerster (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Grant Henke, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/10880

to look at the new patch set (#6).

Change subject: Read url-encoded table name on restore
......................................................................

Read url-encoded table name on restore

Kudu table names are being url-encoded on backup. Read the url-encoded
table names on restore.

Change-Id: Ib028b9239c9e4aa71084757eb2dc16a3068de929
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
3 files changed, 21 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/10880/6
-- 
To view, visit http://gerrit.cloudera.org:8080/10880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib028b9239c9e4aa71084757eb2dc16a3068de929
Gerrit-Change-Number: 10880
Gerrit-PatchSet: 6
Gerrit-Owner: Tony Foerster <to...@phdata.io>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tony Foerster <to...@phdata.io>

[kudu-CR] Read url-enconded hdfs path on restore

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10880 )

Change subject: Read url-enconded hdfs path on restore
......................................................................


Patch Set 3:

(3 comments)

Looks good, just want some clarifications on comments.

http://gerrit.cloudera.org:8080/#/c/10880/3//COMMIT_MSG
Commit Message:

PS3: 
IIUC, the patch ensures that the _table name_ is URL-encoded when resolved during restore; previously it wasn't. Yet, this commit message talks about the HDFS path. Could you clarify?


http://gerrit.cloudera.org:8080/#/c/10880/3//COMMIT_MSG@7
PS3, Line 7: enconded
encoded


http://gerrit.cloudera.org:8080/#/c/10880/3/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
File java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala:

http://gerrit.cloudera.org:8080/#/c/10880/3/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@44
PS3, Line 44:   // Simulate an Impala table with special characters
What's the effect of this override? What different code path is being tested? Could you add a little more detail to the comment?



-- 
To view, visit http://gerrit.cloudera.org:8080/10880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib028b9239c9e4aa71084757eb2dc16a3068de929
Gerrit-Change-Number: 10880
Gerrit-PatchSet: 3
Gerrit-Owner: Tony Foerster <to...@phdata.io>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 06 Jul 2018 17:58:14 +0000
Gerrit-HasComments: Yes

[kudu-CR] Read url-encoded table name on restore

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10880 )

Change subject: Read url-encoded table name on restore
......................................................................

Read url-encoded table name on restore

Kudu table names are being url-encoded on backup. Read the url-encoded
table names on restore.

Change-Id: Ib028b9239c9e4aa71084757eb2dc16a3068de929
Reviewed-on: http://gerrit.cloudera.org:8080/10880
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Adar Dembo <ad...@cloudera.com>
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduRestore.scala
M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
3 files changed, 21 insertions(+), 3 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved; Verified

-- 
To view, visit http://gerrit.cloudera.org:8080/10880
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib028b9239c9e4aa71084757eb2dc16a3068de929
Gerrit-Change-Number: 10880
Gerrit-PatchSet: 7
Gerrit-Owner: Tony Foerster <to...@phdata.io>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Tony Foerster <to...@phdata.io>