You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Zoltan Chovan (Code Review)" <ge...@cloudera.org> on 2021/12/07 16:59:15 UTC
[kudu-CR] [java] KUDU-2623 Expose table as public in WriteOp
Zoltan Chovan has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18075
Change subject: [java] KUDU-2623 Expose table as public in WriteOp
......................................................................
[java] KUDU-2623 Expose table as public in WriteOp
Making KuduWriteOperation.table() method public to enable identification
of the problematic table when an error happens.
Change-Id: Idbfff6e931304cce7826bc272811155f57a5c4e5
---
M src/kudu/client/client-test.cc
M src/kudu/client/write_op.h
2 files changed, 35 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/18075/1
--
To view, visit http://gerrit.cloudera.org:8080/18075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Idbfff6e931304cce7826bc272811155f57a5c4e5
Gerrit-Change-Number: 18075
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [java] KUDU-2623 Expose table as public in WriteOp
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18075 )
Change subject: [java] KUDU-2623 Expose table as public in WriteOp
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18075/2/src/kudu/client/write_op.h
File src/kudu/client/write_op.h:
http://gerrit.cloudera.org:8080/#/c/18075/2/src/kudu/client/write_op.h@97
PS2, Line 97: const KuduTable* table() const { return table_.get(); }
> I haven't looked into it previously. This seemed the easiest/least intrusiv
Also, the review/gerrit link is broken and I wasn't able to locate the change on gerrit. There's no hit for the change id or the commit title.
--
To view, visit http://gerrit.cloudera.org:8080/18075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idbfff6e931304cce7826bc272811155f57a5c4e5
Gerrit-Change-Number: 18075
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Dec 2021 18:06:46 +0000
Gerrit-HasComments: Yes
[kudu-CR] [client] KUDU-2623 Expose table as public in WriteOp
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18075 )
Change subject: [client] KUDU-2623 Expose table as public in WriteOp
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/18075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idbfff6e931304cce7826bc272811155f57a5c4e5
Gerrit-Change-Number: 18075
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 Dec 2021 08:45:22 +0000
Gerrit-HasComments: No
[kudu-CR] [java] KUDU-2623 Expose table as public in WriteOp
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18075 )
Change subject: [java] KUDU-2623 Expose table as public in WriteOp
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/18075/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18075/2//COMMIT_MSG@7
PS2, Line 7: java
Why java? I might be missing something, but didn't find anything java-specific in this patch.
http://gerrit.cloudera.org:8080/#/c/18075/2//COMMIT_MSG@9
PS2, Line 9: KuduWriteOperation.table
nit: in C++ notation that would be KuduWriteOperation::table()
http://gerrit.cloudera.org:8080/#/c/18075/2/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/18075/2/src/kudu/client/client-test.cc@3539
PS2, Line 3539: const char*
nit: could be
constexpr const char* const ...
http://gerrit.cloudera.org:8080/#/c/18075/2/src/kudu/client/client-test.cc@3561
PS2, Line 3561: &overflow
Since 'overflow' isn't used in the code below, it's possible to pass 'nullptr' here instead.
http://gerrit.cloudera.org:8080/#/c/18075/2/src/kudu/client/write_op.h
File src/kudu/client/write_op.h:
http://gerrit.cloudera.org:8080/#/c/18075/2/src/kudu/client/write_op.h@97
PS2, Line 97: const KuduTable* table() const { return table_.get(); }
It seems in 95dd95fde5d550f5ce8374f440e850d2cc722bf4 this has been moved into the private part of the interface. Just to double-check: did you check the story behind that changelist?
--
To view, visit http://gerrit.cloudera.org:8080/18075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idbfff6e931304cce7826bc272811155f57a5c4e5
Gerrit-Change-Number: 18075
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Dec 2021 17:50:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] [client] KUDU-2623 Expose table as public in WriteOp
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18075 )
Change subject: [client] KUDU-2623 Expose table as public in WriteOp
......................................................................
Patch Set 3:
> Patch Set 3: Code-Review+2
>
> LGTM. Please make sure that somewhat similar approach works for the Java client as well -- the idea is that ideally this have to almost 'mirror' the approach in Java client.
Thank you!
I did check the Java client, there the table object is publicly accessible, e.g.:
RowErrorsAndOverflowStatus reos = session.getPendingErrors();
reos.getRowErrors()[0].getOperation().getTable().getName();
I'm still working on confirming this on the python client.
--
To view, visit http://gerrit.cloudera.org:8080/18075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idbfff6e931304cce7826bc272811155f57a5c4e5
Gerrit-Change-Number: 18075
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 Dec 2021 17:39:43 +0000
Gerrit-HasComments: No
[kudu-CR] [client] KUDU-2623 Expose table as public in WriteOp
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Attila Bukor, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18075
to look at the new patch set (#3).
Change subject: [client] KUDU-2623 Expose table as public in WriteOp
......................................................................
[client] KUDU-2623 Expose table as public in WriteOp
Making KuduWriteOperation::table() method public to enable identification
of the problematic table when an error happens.
Change-Id: Idbfff6e931304cce7826bc272811155f57a5c4e5
---
M src/kudu/client/client-test.cc
M src/kudu/client/write_op.h
2 files changed, 34 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/18075/3
--
To view, visit http://gerrit.cloudera.org:8080/18075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idbfff6e931304cce7826bc272811155f57a5c4e5
Gerrit-Change-Number: 18075
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [java] KUDU-2623 Expose table as public in WriteOp
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Attila Bukor, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18075
to look at the new patch set (#2).
Change subject: [java] KUDU-2623 Expose table as public in WriteOp
......................................................................
[java] KUDU-2623 Expose table as public in WriteOp
Making KuduWriteOperation.table() method public to enable identification
of the problematic table when an error happens.
Change-Id: Idbfff6e931304cce7826bc272811155f57a5c4e5
---
M src/kudu/client/client-test.cc
M src/kudu/client/write_op.h
2 files changed, 35 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/18075/2
--
To view, visit http://gerrit.cloudera.org:8080/18075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idbfff6e931304cce7826bc272811155f57a5c4e5
Gerrit-Change-Number: 18075
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
[kudu-CR] [java] KUDU-2623 Expose table as public in WriteOp
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18075 )
Change subject: [java] KUDU-2623 Expose table as public in WriteOp
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/18075/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18075/2//COMMIT_MSG@7
PS2, Line 7: java
> Why java? I might be missing something, but didn't find anything java-spec
yeah, that was a brain fart, I meant client
http://gerrit.cloudera.org:8080/#/c/18075/2/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/18075/2/src/kudu/client/client-test.cc@3539
PS2, Line 3539: const char*
> nit: could be
Ack
http://gerrit.cloudera.org:8080/#/c/18075/2/src/kudu/client/client-test.cc@3561
PS2, Line 3561: &overflow
> Since 'overflow' isn't used in the code below, it's possible to pass 'nullp
Ack
http://gerrit.cloudera.org:8080/#/c/18075/2/src/kudu/client/write_op.h
File src/kudu/client/write_op.h:
http://gerrit.cloudera.org:8080/#/c/18075/2/src/kudu/client/write_op.h@97
PS2, Line 97: const KuduTable* table() const { return table_.get(); }
> It seems in 95dd95fde5d550f5ce8374f440e850d2cc722bf4 this has been moved in
I haven't looked into it previously. This seemed the easiest/least intrusive way to expose this information so that it's available during error processing in the client.
To be honest I'm not sure if I fully understand the implications of 95dd95fde5d550f5ce8374f440e850d2cc722bf4
I ran the whole test suite and didn't break anything, so please let me know if this breaks something in other ways.
--
To view, visit http://gerrit.cloudera.org:8080/18075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idbfff6e931304cce7826bc272811155f57a5c4e5
Gerrit-Change-Number: 18075
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Dec 2021 18:03:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] [java] KUDU-2623 Expose table as public in WriteOp
Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18075 )
Change subject: [java] KUDU-2623 Expose table as public in WriteOp
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18075/2/src/kudu/client/write_op.h
File src/kudu/client/write_op.h:
http://gerrit.cloudera.org:8080/#/c/18075/2/src/kudu/client/write_op.h@97
PS2, Line 97: const KuduTable* table() const { return table_.get(); }
> Also, the review/gerrit link is broken and I wasn't able to locate the chan
Nvm, found it, but it doesn't contain any additional info.
--
To view, visit http://gerrit.cloudera.org:8080/18075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idbfff6e931304cce7826bc272811155f57a5c4e5
Gerrit-Change-Number: 18075
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Dec 2021 18:26:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] [client] KUDU-2623 Expose table as public in WriteOp
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18075 )
Change subject: [client] KUDU-2623 Expose table as public in WriteOp
......................................................................
Patch Set 3:
> > Patch Set 3: Code-Review+2
> >
> > LGTM. Please make sure that somewhat similar approach works for
> the Java client as well -- the idea is that ideally this have to
> almost 'mirror' the approach in Java client.
>
> Thank you!
>
> I did check the Java client, there the table object is publicly
> accessible, e.g.:
> RowErrorsAndOverflowStatus reos = session.getPendingErrors();
> reos.getRowErrors()[0].getOperation().getTable().getName();
>
> I'm still working on confirming this on the python client.
Ah, cool: thank you for clarifying on this!
--
To view, visit http://gerrit.cloudera.org:8080/18075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idbfff6e931304cce7826bc272811155f57a5c4e5
Gerrit-Change-Number: 18075
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 Dec 2021 17:51:52 +0000
Gerrit-HasComments: No
[kudu-CR] [client] KUDU-2623 Expose table as public in WriteOp
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18075 )
Change subject: [client] KUDU-2623 Expose table as public in WriteOp
......................................................................
[client] KUDU-2623 Expose table as public in WriteOp
Making KuduWriteOperation::table() method public to enable identification
of the problematic table when an error happens.
Change-Id: Idbfff6e931304cce7826bc272811155f57a5c4e5
Reviewed-on: http://gerrit.cloudera.org:8080/18075
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Attila Bukor <ab...@apache.org>
---
M src/kudu/client/client-test.cc
M src/kudu/client/write_op.h
2 files changed, 34 insertions(+), 2 deletions(-)
Approvals:
Kudu Jenkins: Verified
Alexey Serbin: Looks good to me, approved
Attila Bukor: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/18075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Idbfff6e931304cce7826bc272811155f57a5c4e5
Gerrit-Change-Number: 18075
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>