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

[kudu-CR] [thirdparty] allow different URL prefix 

Hao Hao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11275


Change subject: [thirdparty] allow different URL prefix
......................................................................

[thirdparty] allow different URL prefix

This patch adds the option to allow different URL prefix when
downloading third party dependencies.

Change-Id: I004828785d9b0d484806d743d7a1084a3ffd576b
---
M thirdparty/download-thirdparty.sh
1 file changed, 41 insertions(+), 19 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I004828785d9b0d484806d743d7a1084a3ffd576b
Gerrit-Change-Number: 11275
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>

[kudu-CR] [thirdparty] allow different URL prefix

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins, Adar Dembo, 

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

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

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

Change subject: [thirdparty] allow different URL prefix
......................................................................

[thirdparty] allow different URL prefix

This patch adds the option to allow different URL prefix when
downloading third party dependencies.

Change-Id: I004828785d9b0d484806d743d7a1084a3ffd576b
---
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
2 files changed, 47 insertions(+), 19 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I004828785d9b0d484806d743d7a1084a3ffd576b
Gerrit-Change-Number: 11275
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [thirdparty] allow different URL prefix

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

Change subject: [thirdparty] allow different URL prefix
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11275/1/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:

http://gerrit.cloudera.org:8080/#/c/11275/1/thirdparty/download-thirdparty.sh@58
PS1, Line 58:   local URL_PREFIX=$3
> I don't think it makes sense to parameterize this, because it's probably go
I misunderstood Hao's use case: she does want to download some dependencies from an alternative URL, and the rest from the usual Cloudfront URL. So parameterization is necessary.


http://gerrit.cloudera.org:8080/#/c/11275/1/thirdparty/download-thirdparty.sh@129
PS1, Line 129: fetch_with_url_and_patch() {
Could we avoid needing both fetch_with_url_and_patch and fetch_and_patch by adding URL_PREFIX as the (optional) last argument to fetch_and_patch? With a good comment I think it'll be pretty obvious how one should use the function.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I004828785d9b0d484806d743d7a1084a3ffd576b
Gerrit-Change-Number: 11275
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 20 Aug 2018 22:04:58 +0000
Gerrit-HasComments: Yes

[kudu-CR] [thirdparty] allow different URL prefix

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

Change subject: [thirdparty] allow different URL prefix
......................................................................


Patch Set 3: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I004828785d9b0d484806d743d7a1084a3ffd576b
Gerrit-Change-Number: 11275
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 23 Aug 2018 22:58:13 +0000
Gerrit-HasComments: No

[kudu-CR] [thirdparty] allow different URL prefix

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

Change subject: [thirdparty] allow different URL prefix
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11275/2/thirdparty/vars.sh
File thirdparty/vars.sh:

http://gerrit.cloudera.org:8080/#/c/11275/2/thirdparty/vars.sh@35
PS2, Line 35: DEPENDENCY_URL=${DEPENDENCY_URL:-$CLOUDFRONT_URL_PREFIX}
> Set this up like this:
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I004828785d9b0d484806d743d7a1084a3ffd576b
Gerrit-Change-Number: 11275
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 22 Aug 2018 18:36:05 +0000
Gerrit-HasComments: Yes

[kudu-CR] [thirdparty] allow different URL prefix

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

Change subject: [thirdparty] allow different URL prefix
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11275/1/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:

http://gerrit.cloudera.org:8080/#/c/11275/1/thirdparty/download-thirdparty.sh@129
PS1, Line 129: fetch_with_url_and_patch() {
> The problem here is fetch_and_patch already has an optional argument for 'C
Ah good point. OK, so let's just focus on making fetch_and_patch simpler. Perhaps like this?

  // Call fetch_with_url_and_patch with the default dependency URL source.
  fetch_and_patch() {
    fetch_with_url_and_patch $1 $2 $3 $DEP_URL "$@"
  }

(I still think CLOUDFRONT_URL_PREFIX should be read-only and there should be a new variable to represent the URL that defaults to CLOUDFRONT_URL_PREFIX but may be overridden/patched).



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I004828785d9b0d484806d743d7a1084a3ffd576b
Gerrit-Change-Number: 11275
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 21 Aug 2018 04:14:24 +0000
Gerrit-HasComments: Yes

[kudu-CR] [thirdparty] allow different URL prefix

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

Change subject: [thirdparty] allow different URL prefix
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11275/1/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:

http://gerrit.cloudera.org:8080/#/c/11275/1/thirdparty/download-thirdparty.sh@58
PS1, Line 58:   local URL_PREFIX=$3
I don't think it makes sense to parameterize this, because it's probably going to be the same for _all_ dependencies in a given run of download-thirdparty.sh.

Instead, I would add indirection to the URL prefixing variables. Maybe something like:
1. Keep CLOUDFRONT_URL_PREFIX as a read-only variable with the URL for Cloudfront-based downloads.
2. Introduce a new URL variable in vars.sh which defaults to CLOUDFRONT_URL_PREFIX.
3. Use this new variable as the download source in download-thirdparty.sh.

Then anyone who wants to use a different download source need only override that new URL variable in the environment, or patch vars.sh to set it to some other value.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I004828785d9b0d484806d743d7a1084a3ffd576b
Gerrit-Change-Number: 11275
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 20 Aug 2018 19:42:57 +0000
Gerrit-HasComments: Yes

[kudu-CR] [thirdparty] allow different URL prefix

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

Change subject: [thirdparty] allow different URL prefix
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11275/2/thirdparty/vars.sh
File thirdparty/vars.sh:

http://gerrit.cloudera.org:8080/#/c/11275/2/thirdparty/vars.sh@35
PS2, Line 35: DEPENDENCY_URL=$CLOUDFRONT_URL_PREFIX
Set this up like this:

  DEPENDENCY_URL=${DEPENDENCY_URL:-$CLOUDFRONT_URL_PREFIX}

That way DEPENDENCY_URL can be overridden via environment variable too.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I004828785d9b0d484806d743d7a1084a3ffd576b
Gerrit-Change-Number: 11275
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 22 Aug 2018 03:37:11 +0000
Gerrit-HasComments: Yes

[kudu-CR] [thirdparty] allow different URL prefix

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

Change subject: [thirdparty] allow different URL prefix
......................................................................


Patch Set 3: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I004828785d9b0d484806d743d7a1084a3ffd576b
Gerrit-Change-Number: 11275
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 22 Aug 2018 19:22:59 +0000
Gerrit-HasComments: No

[kudu-CR] [thirdparty] allow different URL prefix

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

Change subject: [thirdparty] allow different URL prefix
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11275/1/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:

http://gerrit.cloudera.org:8080/#/c/11275/1/thirdparty/download-thirdparty.sh@129
PS1, Line 129: fetch_with_url_and_patch() {
> Could we avoid needing both fetch_with_url_and_patch and fetch_and_patch by
The problem here is fetch_and_patch already has an optional argument for 'COMMANDS'. I do not see a clean way to get away with that. Do you have some suggestions? Thanks!



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I004828785d9b0d484806d743d7a1084a3ffd576b
Gerrit-Change-Number: 11275
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 21 Aug 2018 00:49:06 +0000
Gerrit-HasComments: Yes

[kudu-CR] [thirdparty] allow different URL prefix

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

Change subject: [thirdparty] allow different URL prefix
......................................................................

[thirdparty] allow different URL prefix

This patch adds the option to allow different URL prefix when
downloading third party dependencies.

Change-Id: I004828785d9b0d484806d743d7a1084a3ffd576b
Reviewed-on: http://gerrit.cloudera.org:8080/11275
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert <da...@apache.org>
---
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
2 files changed, 47 insertions(+), 19 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified
  Dan Burkert: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I004828785d9b0d484806d743d7a1084a3ffd576b
Gerrit-Change-Number: 11275
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [thirdparty] allow different URL prefix

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

Change subject: [thirdparty] allow different URL prefix
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11275/1/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:

http://gerrit.cloudera.org:8080/#/c/11275/1/thirdparty/download-thirdparty.sh@129
PS1, Line 129: 
> Ah good point. OK, so let's just focus on making fetch_and_patch simpler. P
Thanks for the suggestion. Updated, but I don't think the above line will work without shift the arguments, as '$@' means all of the parameters passed to the function.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I004828785d9b0d484806d743d7a1084a3ffd576b
Gerrit-Change-Number: 11275
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 22 Aug 2018 00:12:29 +0000
Gerrit-HasComments: Yes

[kudu-CR] [thirdparty] allow different URL prefix

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins, Adar Dembo, 

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

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

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

Change subject: [thirdparty] allow different URL prefix
......................................................................

[thirdparty] allow different URL prefix

This patch adds the option to allow different URL prefix when
downloading third party dependencies.

Change-Id: I004828785d9b0d484806d743d7a1084a3ffd576b
---
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
2 files changed, 47 insertions(+), 19 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I004828785d9b0d484806d743d7a1084a3ffd576b
Gerrit-Change-Number: 11275
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins