You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Attila Bukor (Code Review)" <ge...@cloudera.org> on 2020/07/20 14:56:30 UTC

[kudu-CR] [python] KUDU-3090 Add support for table ownership 

Attila Bukor has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16217


Change subject: [python] KUDU-3090 Add support for table ownership
......................................................................

[python] KUDU-3090 Add support for table ownership

Change-Id: I1c1e12a94459e0f04c769d38d96bffb12f337bda
---
M python/kudu/client.pyx
M python/kudu/libkudu_client.pxd
M python/kudu/tests/test_client.py
3 files changed, 56 insertions(+), 1 deletion(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1c1e12a94459e0f04c769d38d96bffb12f337bda
Gerrit-Change-Number: 16217
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>

[kudu-CR] [python] KUDU-3090 Add support for table ownership

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

Change subject: [python] KUDU-3090 Add support for table ownership
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/16217/1/python/kudu/tests/test_client.py
File python/kudu/tests/test_client.py:

http://gerrit.cloudera.org:8080/#/c/16217/1/python/kudu/tests/test_client.py@157
PS1, Line 157:         name = 'table_with_different_owner'
             :         try:
             :             self.client.create_table(
             :                     name, self.schema,
             :                     partitioning=Partitioning().add_hash_partitions(['key'], 2),
             :                     owner='alice')
             : 
             :             self.assertEqual('alice', self.client.table(name).owner)
             : 
> Can we also open the table anew and check the owner is what we expect?
Ah actually ignore this; client.table() is already opening the table anew.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c1e12a94459e0f04c769d38d96bffb12f337bda
Gerrit-Change-Number: 16217
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 20 Jul 2020 22:49:42 +0000
Gerrit-HasComments: Yes

[kudu-CR] [python] KUDU-3090 Add support for table ownership

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

Change subject: [python] KUDU-3090 Add support for table ownership
......................................................................


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/16217/1/python/kudu/tests/test_client.py
File python/kudu/tests/test_client.py:

http://gerrit.cloudera.org:8080/#/c/16217/1/python/kudu/tests/test_client.py@162
PS1, Line 162:                     owner='alice')
Can you also add a test that doesn't set this (or calls this with owner=None), that checks what we get an owner still?


http://gerrit.cloudera.org:8080/#/c/16217/1/python/kudu/tests/test_client.py@157
PS1, Line 157:         name = 'table_with_different_owner'
             :         try:
             :             self.client.create_table(
             :                     name, self.schema,
             :                     partitioning=Partitioning().add_hash_partitions(['key'], 2),
             :                     owner='alice')
             : 
             :             self.assertEqual('alice', self.client.table(name).owner)
             : 
Can we also open the table anew and check the owner is what we expect?


http://gerrit.cloudera.org:8080/#/c/16217/1/python/kudu/tests/test_client.py@375
PS1, Line 375:             table = alterer.set_owner('alice').alter()
What happens if someone tries set_owner(None)? Can you add that as a test?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c1e12a94459e0f04c769d38d96bffb12f337bda
Gerrit-Change-Number: 16217
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 20 Jul 2020 22:48:42 +0000
Gerrit-HasComments: Yes

[kudu-CR] [python] KUDU-3090 Add support for table ownership

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has removed a vote on this change.

Change subject: [python] KUDU-3090 Add support for table ownership
......................................................................


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I1c1e12a94459e0f04c769d38d96bffb12f337bda
Gerrit-Change-Number: 16217
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [python] KUDU-3090 Add support for table ownership

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

Change subject: [python] KUDU-3090 Add support for table ownership
......................................................................


Patch Set 1: Verified+1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c1e12a94459e0f04c769d38d96bffb12f337bda
Gerrit-Change-Number: 16217
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 20 Jul 2020 15:32:43 +0000
Gerrit-HasComments: No

[kudu-CR] [python] KUDU-3090 Add support for table ownership

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Grant Henke, 

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

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

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

Change subject: [python] KUDU-3090 Add support for table ownership
......................................................................

[python] KUDU-3090 Add support for table ownership

Change-Id: I1c1e12a94459e0f04c769d38d96bffb12f337bda
---
M python/kudu/client.pyx
M python/kudu/libkudu_client.pxd
M python/kudu/tests/test_client.py
3 files changed, 59 insertions(+), 1 deletion(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1c1e12a94459e0f04c769d38d96bffb12f337bda
Gerrit-Change-Number: 16217
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [python] KUDU-3090 Add support for table ownership

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

Change subject: [python] KUDU-3090 Add support for table ownership
......................................................................

[python] KUDU-3090 Add support for table ownership

Change-Id: I1c1e12a94459e0f04c769d38d96bffb12f337bda
Reviewed-on: http://gerrit.cloudera.org:8080/16217
Tested-by: Kudu Jenkins
Reviewed-by: Grant Henke <gr...@apache.org>
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
M python/kudu/client.pyx
M python/kudu/libkudu_client.pxd
M python/kudu/tests/test_client.py
3 files changed, 59 insertions(+), 1 deletion(-)

Approvals:
  Kudu Jenkins: Verified
  Grant Henke: Looks good to me, but someone else must approve
  Andrew Wong: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I1c1e12a94459e0f04c769d38d96bffb12f337bda
Gerrit-Change-Number: 16217
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] [python] KUDU-3090 Add support for table ownership

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

Change subject: [python] KUDU-3090 Add support for table ownership
......................................................................


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/16217/1/python/kudu/tests/test_client.py
File python/kudu/tests/test_client.py:

http://gerrit.cloudera.org:8080/#/c/16217/1/python/kudu/tests/test_client.py@162
PS1, Line 162:                     owner='alice')
> Can you also add a test that doesn't set this (or calls this with owner=Non
added a check to test_table_basics


http://gerrit.cloudera.org:8080/#/c/16217/1/python/kudu/tests/test_client.py@157
PS1, Line 157:         name = 'table_with_different_owner'
             :         try:
             :             self.client.create_table(
             :                     name, self.schema,
             :                     partitioning=Partitioning().add_hash_partitions(['key'], 2),
             :                     owner='alice')
             : 
             :             self.assertEqual('alice', self.client.table(name).owner)
             : 
> Ah actually ignore this; client.table() is already opening the table anew.
Done


http://gerrit.cloudera.org:8080/#/c/16217/1/python/kudu/tests/test_client.py@375
PS1, Line 375:             table = alterer.set_owner('alice').alter()
> What happens if someone tries set_owner(None)? Can you add that as a test?
It throws a TypeError as we don't check for None-ness here. Do you think it's worth adding this as a test? We don't have a test for renaming table to None either.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c1e12a94459e0f04c769d38d96bffb12f337bda
Gerrit-Change-Number: 16217
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 21 Jul 2020 08:42:57 +0000
Gerrit-HasComments: Yes

[kudu-CR] [python] KUDU-3090 Add support for table ownership

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

Change subject: [python] KUDU-3090 Add support for table ownership
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/16217/1/python/kudu/tests/test_client.py
File python/kudu/tests/test_client.py:

http://gerrit.cloudera.org:8080/#/c/16217/1/python/kudu/tests/test_client.py@375
PS1, Line 375:             table = alterer.set_owner('alice').alter()
> It throws a TypeError as we don't check for None-ness here. Do you think it
This matches the other clients behavior which doesn't allow setting an empty owner because we expect all tables, except legacy tables, will have an owner.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c1e12a94459e0f04c769d38d96bffb12f337bda
Gerrit-Change-Number: 16217
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 21 Jul 2020 12:55:41 +0000
Gerrit-HasComments: Yes

[kudu-CR] [python] KUDU-3090 Add support for table ownership

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

Change subject: [python] KUDU-3090 Add support for table ownership
......................................................................


Patch Set 2: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/16217/1/python/kudu/tests/test_client.py
File python/kudu/tests/test_client.py:

http://gerrit.cloudera.org:8080/#/c/16217/1/python/kudu/tests/test_client.py@375
PS1, Line 375:             alterer = self.client.new_table_alterer(table)
> This matches the other clients behavior which doesn't allow setting an empt
Thinking about the alterer as a black box, it seemed possible that this would leave the table name alone and succeed. As well it seems to be more acceptable to my Python brain to pass None around, at least moreso than passing nullptr around as a ref in C++. Thanks for adding this!



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c1e12a94459e0f04c769d38d96bffb12f337bda
Gerrit-Change-Number: 16217
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 21 Jul 2020 17:56:24 +0000
Gerrit-HasComments: Yes

[kudu-CR] [python] KUDU-3090 Add support for table ownership

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

Change subject: [python] KUDU-3090 Add support for table ownership
......................................................................


Patch Set 2: Code-Review+1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c1e12a94459e0f04c769d38d96bffb12f337bda
Gerrit-Change-Number: 16217
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 21 Jul 2020 12:55:47 +0000
Gerrit-HasComments: No