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 2019/10/31 14:12:14 UTC
[kudu-CR] [util] Fix comment of faststring
lingbinlb@gmail.com has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14604
Change subject: [util] Fix comment of faststring
......................................................................
[util] Fix comment of faststring
1. Under any case, capacity() is larger than the data length.
If the current length is shorter than kInitialCapacity, after call
shrink_to_fit(), capacity() will go down to kInitialCapacity.
2. The caller must be responsible for releaseing the pointer release() returns.
Change-Id: Ie4bd4f31216fdf5cfbf70526f1358824fbda225a
---
M src/kudu/util/faststring.h
1 file changed, 8 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/14604/1
--
To view, visit http://gerrit.cloudera.org:8080/14604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie4bd4f31216fdf5cfbf70526f1358824fbda225a
Gerrit-Change-Number: 14604
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
[kudu-CR] [util] Fix comment of faststring
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14604 )
Change subject: [util] Fix comment of faststring
......................................................................
Patch Set 5: Verified+1 Code-Review+2
The TSAN failure is unrelated to your change, so we can ignore the -1, especially since you got a +1 earlier and nothing significant changed.
--
To view, visit http://gerrit.cloudera.org:8080/14604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie4bd4f31216fdf5cfbf70526f1358824fbda225a
Gerrit-Change-Number: 14604
Gerrit-PatchSet: 5
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 01 Nov 2019 20:57:34 +0000
Gerrit-HasComments: No
[kudu-CR] [util] Fix comment of faststring
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
lingbinlb@gmail.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/14604 )
Change subject: [util] Fix comment of faststring
......................................................................
Patch Set 5:
> Patch Set 5: Verified-1
>
> Build Failed
>
> http://jenkins.kudu.apache.org/job/kudu-gerrit/19397/ : FAILURE
It seems that Jenkins has encountered a problem with `TSAN`. Can you give me some tips on how to solve this problem?
--
To view, visit http://gerrit.cloudera.org:8080/14604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie4bd4f31216fdf5cfbf70526f1358824fbda225a
Gerrit-Change-Number: 14604
Gerrit-PatchSet: 5
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 01 Nov 2019 16:18:23 +0000
Gerrit-HasComments: No
[kudu-CR] [util] Fix comment of faststring
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
lingbinlb@gmail.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/14604 )
Change subject: [util] Fix comment of faststring
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14604/4/src/kudu/util/faststring-test.cc
File src/kudu/util/faststring-test.cc:
http://gerrit.cloudera.org:8080/#/c/14604/4/src/kudu/util/faststring-test.cc@93
PS4, Line 93: std::string
> Could you add "#include <string>", "using std::string", then remove these s
Done
--
To view, visit http://gerrit.cloudera.org:8080/14604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie4bd4f31216fdf5cfbf70526f1358824fbda225a
Gerrit-Change-Number: 14604
Gerrit-PatchSet: 4
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 01 Nov 2019 10:00:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Fix comment of faststring
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14604 )
Change subject: [util] Fix comment of faststring
......................................................................
Patch Set 3:
(3 comments)
Could you add a few tests to faststring-test that verify the growth behavior you want? Should be as simple as a few append() calls mixed in with verifications of length() and capacity().
http://gerrit.cloudera.org:8080/#/c/14604/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14604/3//COMMIT_MSG@9
PS3, Line 9: larger than
Not to be overly pedantic, "greater than or equal to" is more accurate, right?
http://gerrit.cloudera.org:8080/#/c/14604/3//COMMIT_MSG@13
PS3, Line 13: releaseing
Nit: releasing
http://gerrit.cloudera.org:8080/#/c/14604/3//COMMIT_MSG@20
PS3, Line 20: This
this
--
To view, visit http://gerrit.cloudera.org:8080/14604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie4bd4f31216fdf5cfbf70526f1358824fbda225a
Gerrit-Change-Number: 14604
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 01 Nov 2019 07:42:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Fix comment of faststring
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14604 )
Change subject: [util] Fix comment of faststring
......................................................................
[util] Fix comment of faststring
1. Under any case, capacity() is greater than or equal to
the data length. If the current length is shorter than
kInitialCapacity, after call shrink_to_fit(),
capacity() will go down to kInitialCapacity.
2. The caller must be responsible for releasing the pointer
release() returns.
Now In `GrowByAtLeast()`, to_reserve (i.e. `len_ + count`)
is usually larger than we really need. This commit also
calculate an exact value when we try to expand the faststring.
To be clearer, this commit rename `GrowByAtLeast()` to
`GrowToAtLeast()`, and make the newcapacity as its argument.
Note: If the capacity needs to grow, it still expand by
at least 50% each time.
Change-Id: Ie4bd4f31216fdf5cfbf70526f1358824fbda225a
Reviewed-on: http://gerrit.cloudera.org:8080/14604
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/util/faststring-test.cc
M src/kudu/util/faststring.cc
M src/kudu/util/faststring.h
3 files changed, 76 insertions(+), 13 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/14604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie4bd4f31216fdf5cfbf70526f1358824fbda225a
Gerrit-Change-Number: 14604
Gerrit-PatchSet: 6
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@gmail.com>
[kudu-CR] [util] Fix comment of faststring
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
lingbinlb@gmail.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/14604 )
Change subject: [util] Fix comment of faststring
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14604/1/src/kudu/util/faststring.h
File src/kudu/util/faststring.h:
http://gerrit.cloudera.org:8080/#/c/14604/1/src/kudu/util/faststring.h@239
PS1, Line 239: GrowByAtLeast(len_ + count - capacity_);
> There were also a lot of non-Python failures: http://dist-test.cloudera.org
I know the reason, because in `GrowByAtLeast()`, `to_reserve` should be equal to `capacity_ + count`, instead of `len_ + count`.
For example: when capacity_=32, len_=10, if you need `Append()` with a length of 40, then the memory size we need to expand should be at least (10 + 40 - 32 = 18), and In the original implementation, `len_ + count` was used as the total capacity, which is 10 + 18 = 28, which is wrong. It should be `capacity + count`, ie 32+18 = 50
--
To view, visit http://gerrit.cloudera.org:8080/14604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie4bd4f31216fdf5cfbf70526f1358824fbda225a
Gerrit-Change-Number: 14604
Gerrit-PatchSet: 2
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 01 Nov 2019 06:48:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Fix comment of faststring
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14604
to look at the new patch set (#4).
Change subject: [util] Fix comment of faststring
......................................................................
[util] Fix comment of faststring
1. Under any case, capacity() is greater than or equal to
the data length. If the current length is shorter than
kInitialCapacity, after call shrink_to_fit(),
capacity() will go down to kInitialCapacity.
2. The caller must be responsible for releasing the pointer
release() returns.
Now In `GrowByAtLeast()`, to_reserve (i.e. `len_ + count`)
is usually larger than we really need. This commit also
calculate an exact value when we try to expand the faststring.
To be clearer, this commit rename `GrowByAtLeast()` to
`GrowToAtLeast()`, and make the newcapacity as its argument.
Note: If the capacity needs to grow, it still expand by
at least 50% each time.
Change-Id: Ie4bd4f31216fdf5cfbf70526f1358824fbda225a
---
M src/kudu/util/faststring-test.cc
M src/kudu/util/faststring.cc
M src/kudu/util/faststring.h
3 files changed, 71 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/14604/4
--
To view, visit http://gerrit.cloudera.org:8080/14604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie4bd4f31216fdf5cfbf70526f1358824fbda225a
Gerrit-Change-Number: 14604
Gerrit-PatchSet: 4
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [util] Fix comment of faststring
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14604
to look at the new patch set (#2).
Change subject: [util] Fix comment of faststring
......................................................................
[util] Fix comment of faststring
1. Under any case, capacity() is larger than the data length.
If the current length is shorter than kInitialCapacity,
after call shrink_to_fit(), capacity() will go down to
kInitialCapacity.
2. The caller must be responsible for releaseing the pointer
release() returns.
This commit also fixes the argument to call `GrowByAtLeast()`,
passing the argument to be an exact space to expand.
Note: If the capacity needs to grow, it still expand by
at least 50% each time.
Change-Id: Ie4bd4f31216fdf5cfbf70526f1358824fbda225a
---
M src/kudu/util/faststring.h
1 file changed, 8 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/14604/2
--
To view, visit http://gerrit.cloudera.org:8080/14604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie4bd4f31216fdf5cfbf70526f1358824fbda225a
Gerrit-Change-Number: 14604
Gerrit-PatchSet: 2
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [util] Fix comment of faststring
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14604
to look at the new patch set (#5).
Change subject: [util] Fix comment of faststring
......................................................................
[util] Fix comment of faststring
1. Under any case, capacity() is greater than or equal to
the data length. If the current length is shorter than
kInitialCapacity, after call shrink_to_fit(),
capacity() will go down to kInitialCapacity.
2. The caller must be responsible for releasing the pointer
release() returns.
Now In `GrowByAtLeast()`, to_reserve (i.e. `len_ + count`)
is usually larger than we really need. This commit also
calculate an exact value when we try to expand the faststring.
To be clearer, this commit rename `GrowByAtLeast()` to
`GrowToAtLeast()`, and make the newcapacity as its argument.
Note: If the capacity needs to grow, it still expand by
at least 50% each time.
Change-Id: Ie4bd4f31216fdf5cfbf70526f1358824fbda225a
---
M src/kudu/util/faststring-test.cc
M src/kudu/util/faststring.cc
M src/kudu/util/faststring.h
3 files changed, 76 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/14604/5
--
To view, visit http://gerrit.cloudera.org:8080/14604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie4bd4f31216fdf5cfbf70526f1358824fbda225a
Gerrit-Change-Number: 14604
Gerrit-PatchSet: 5
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [util] Fix comment of faststring
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14604 )
Change subject: [util] Fix comment of faststring
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14604/1/src/kudu/util/faststring.h
File src/kudu/util/faststring.h:
http://gerrit.cloudera.org:8080/#/c/14604/1/src/kudu/util/faststring.h@239
PS1, Line 239: GrowByAtLeast(len_ + count - capacity_);
Looks like this change broke a bunch of tests.
--
To view, visit http://gerrit.cloudera.org:8080/14604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie4bd4f31216fdf5cfbf70526f1358824fbda225a
Gerrit-Change-Number: 14604
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 31 Oct 2019 17:49:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Fix comment of faststring
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14604 )
Change subject: [util] Fix comment of faststring
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14604/1/src/kudu/util/faststring.h
File src/kudu/util/faststring.h:
http://gerrit.cloudera.org:8080/#/c/14604/1/src/kudu/util/faststring.h@239
PS1, Line 239: GrowByAtLeast(len_ + count - capacity_);
> This is a bit weird. I checked several failed test cases, they are all in t
There were also a lot of non-Python failures: http://dist-test.cloudera.org/job?job_id=jenkins-slave.1572531693.17453
Here's one. Basically, the new behavior of GrowByAtLeast can sometimes _shrink_ a faststring.
F1031 14:22:13.662508 4259 faststring.cc:39] Check failed: newcapacity >= capacity_ (2 vs. 32)
*** Check failure stack trace: ***
@ 0x7fab9fccf62d google::LogMessage::Fail() at ??:0
@ 0x7fab9fcd164c google::LogMessage::SendToLog() at ??:0
@ 0x7fab9fccf189 google::LogMessage::Flush() at ??:0
@ 0x7fab9fcd1fdf google::LogMessageFatal::~LogMessageFatal() at ??:0
@ 0x7faba0d9d731 kudu::faststring::GrowArray() at ??:0
@ 0x7fabaf4c70df kudu::faststring::append() at ??:0
@ 0x7faba1c89b89 kudu::KeyEncoderTraits<>::EncodeWithSeparators() at ??:0
@ 0x7faba1c2a8d8 kudu::EncodedKeyBuilder::AddColumnKey() at ??:0
@ 0x7faba1c2a474 kudu::EncodedKey::FromContiguousRow() at ??:0
@ 0x7fabac3fb8f5 kudu::tablet::RowSetKeyProbe::RowSetKeyProbe() at ??:0
@ 0x7fabac3c5ca0 kudu::tablet::Tablet::AcquireLockForOp() at ??:0
@ 0x7fabac3c553f kudu::tablet::Tablet::AcquireRowLocks() at ??:0
@ 0x7fabac4fa751 kudu::tablet::WriteTransaction::Prepare() at ??:0
@ 0x7fabac4e21c1 kudu::tablet::TransactionDriver::Prepare() at ??:0
@ 0x7fabac4e0a56 kudu::tablet::TransactionDriver::PrepareTask() at ??:0
@ 0x7fabac4ec4b6 kudu::internal::InvokeHelper<>::MakeItSo() at ??:0
@ 0x7fabac4ec1ab kudu::internal::Invoker<>::Run() at ??:0
@ 0x7faba0f4ae7e kudu::ThreadPool::DispatchThread() at ??:0
@ 0x7faba0f67a23 boost::_bi::bind_t<>::operator()() at ??:0
@ 0x7faba45438ac boost::function0<>::operator()() at ??:0
@ 0x7faba0f2edfb kudu::Thread::SuperviseThread() at ??:0
@ 0x7faba957a184 start_thread at ??:0
@ 0x7fab9c8e6ffd clone at ??:0
--
To view, visit http://gerrit.cloudera.org:8080/14604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie4bd4f31216fdf5cfbf70526f1358824fbda225a
Gerrit-Change-Number: 14604
Gerrit-PatchSet: 2
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 01 Nov 2019 05:44:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Fix comment of faststring
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
lingbinlb@gmail.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/14604 )
Change subject: [util] Fix comment of faststring
......................................................................
Patch Set 1:
(1 comment)
> Patch Set 1:
>
> (1 comment)
http://gerrit.cloudera.org:8080/#/c/14604/1/src/kudu/util/faststring.h
File src/kudu/util/faststring.h:
http://gerrit.cloudera.org:8080/#/c/14604/1/src/kudu/util/faststring.h@239
PS1, Line 239: GrowByAtLeast(len_ + count - capacity_);
> Looks like this change broke a bunch of tests.
This is a bit weird. I checked several failed test cases, they are all in the python dir, and they all encountered NETWORK_ERROR,
such as
http://jenkins.kudu.apache.org/job/kudu-gerrit/19382/BUILD_TYPE=DEBUG/testReport/junit/kudu.tests.test_client/TestClient/test_alter_column/
and
http://jenkins.kudu.apache.org/job/kudu-gerrit/19382/BUILD_TYPE=DEBUG/testReport/junit/kudu.tests.test_client/TestClient/test_alter_table_add_drop_column/
So it looks like there was an accidental running environment problem, let me trigger jenkins again
--
To view, visit http://gerrit.cloudera.org:8080/14604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie4bd4f31216fdf5cfbf70526f1358824fbda225a
Gerrit-Change-Number: 14604
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 01 Nov 2019 05:18:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Fix comment of faststring
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
lingbinlb@gmail.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/14604 )
Change subject: [util] Fix comment of faststring
......................................................................
Patch Set 3:
(3 comments)
> Patch Set 3:
>
> (3 comments)
>
> Could you add a few tests to faststring-test that verify the growth behavior you want? Should be as simple as a few append() calls mixed in with verifications of length() and capacity().
I will add the test.
http://gerrit.cloudera.org:8080/#/c/14604/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14604/3//COMMIT_MSG@9
PS3, Line 9: larger than
> Not to be overly pedantic, "greater than or equal to" is more accurate, rig
Done
http://gerrit.cloudera.org:8080/#/c/14604/3//COMMIT_MSG@13
PS3, Line 13: releaseing
> Nit: releasing
Done
http://gerrit.cloudera.org:8080/#/c/14604/3//COMMIT_MSG@20
PS3, Line 20: This
> this
Done
--
To view, visit http://gerrit.cloudera.org:8080/14604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie4bd4f31216fdf5cfbf70526f1358824fbda225a
Gerrit-Change-Number: 14604
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 01 Nov 2019 08:29:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Fix comment of faststring
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14604 )
Change subject: [util] Fix comment of faststring
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14604/4/src/kudu/util/faststring-test.cc
File src/kudu/util/faststring-test.cc:
http://gerrit.cloudera.org:8080/#/c/14604/4/src/kudu/util/faststring-test.cc@93
PS4, Line 93: std::string
Could you add "#include <string>", "using std::string", then remove these std:: prefixes?
--
To view, visit http://gerrit.cloudera.org:8080/14604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie4bd4f31216fdf5cfbf70526f1358824fbda225a
Gerrit-Change-Number: 14604
Gerrit-PatchSet: 4
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 01 Nov 2019 09:03:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] Fix comment of faststring
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14604
to look at the new patch set (#3).
Change subject: [util] Fix comment of faststring
......................................................................
[util] Fix comment of faststring
1. Under any case, capacity() is larger than the data length.
If the current length is shorter than kInitialCapacity,
after call shrink_to_fit(), capacity() will go down to
kInitialCapacity.
2. The caller must be responsible for releaseing the pointer
release() returns.
Now In `GrowByAtLeast()`, to_reserve (i.e. `len_ + count`)
is usually larger than we really need. This commit also
calculate an exact value when we try to expand the faststring.
To be clearer, This commit rename `GrowByAtLeast()` to
`GrowToAtLeast()`, and make the newcapacity as its argument.
Note: If the capacity needs to grow, it still expand by
at least 50% each time.
Change-Id: Ie4bd4f31216fdf5cfbf70526f1358824fbda225a
---
M src/kudu/util/faststring.cc
M src/kudu/util/faststring.h
2 files changed, 14 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/14604/3
--
To view, visit http://gerrit.cloudera.org:8080/14604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie4bd4f31216fdf5cfbf70526f1358824fbda225a
Gerrit-Change-Number: 14604
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [util] Fix comment of faststring
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/14604 )
Change subject: [util] Fix comment of faststring
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/14604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Ie4bd4f31216fdf5cfbf70526f1358824fbda225a
Gerrit-Change-Number: 14604
Gerrit-PatchSet: 5
Gerrit-Owner: Anonymous Coward <li...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@gmail.com>