You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org> on 2023/01/26 20:14:19 UTC
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Abhishek Chennaka has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19445
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
KUDU-1945 Update auto incrementing counter during bootstrap
The auto incrementing counter would be reset to zero when the tablet
is being initialized. It is essential to have the correct value of
the counter. There are two cases:
1. WAL segments contain insert operations
In this scenario the WAL segments are replayed and since each insert
operation entry has auto incrementing counter which will be used for
the insert operations present in that entry, as long as we have the
latest insert operation entry in the WAL segments, the auto
incrementing counter is populated correctly during bootstrap.
2. WAL segments do not contain insert operations
In the case, we need to fetch the highest auto incrementing counter
value present in the data which is already flushed and update the
in memory auto incrementing counter appropriately. This patch
accomplishes this task.
Testing has been done manually. Test cases to follow but feedback
would be appreciated on
1. The above understanding in case I am missing something
2. The patch itself
Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
---
M src/kudu/tablet/tablet.cc
1 file changed, 25 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/19445/1
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 1
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
[kudu-CR] [WIP]KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Marton Greber, Alexey Serbin, Kudu Jenkins, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19445
to look at the new patch set (#3).
Change subject: [WIP]KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
[WIP]KUDU-1945 Update auto incrementing counter during bootstrap
The auto incrementing counter would be reset to zero when the tablet
is being initialized. It is essential to have the correct value of
the counter. There are two cases:
1. WAL segments contain insert operations
In this scenario the WAL segments are replayed and since each insert
operation entry has auto incrementing counter which will be used for
the insert operations present in that entry, as long as we have the
latest insert operation entry in the WAL segments, the auto
incrementing counter is populated correctly during bootstrap.
2. WAL segments do not contain insert operations
In the case, we need to fetch the highest auto incrementing counter
value present in the data which is already flushed and update the
in memory auto incrementing counter appropriately. This patch
accomplishes this task.
Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
---
M src/kudu/integration-tests/auto_incrementing-itest.cc
M src/kudu/tablet/tablet.cc
2 files changed, 286 insertions(+), 26 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/19445/3
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 3
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] [WIP]KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Marton Greber, Alexey Serbin, Kudu Jenkins, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19445
to look at the new patch set (#2).
Change subject: [WIP]KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
[WIP]KUDU-1945 Update auto incrementing counter during bootstrap
The auto incrementing counter would be reset to zero when the tablet
is being initialized. It is essential to have the correct value of
the counter. There are two cases:
1. WAL segments contain insert operations
In this scenario the WAL segments are replayed and since each insert
operation entry has auto incrementing counter which will be used for
the insert operations present in that entry, as long as we have the
latest insert operation entry in the WAL segments, the auto
incrementing counter is populated correctly during bootstrap.
2. WAL segments do not contain insert operations
In the case, we need to fetch the highest auto incrementing counter
value present in the data which is already flushed and update the
in memory auto incrementing counter appropriately. This patch
accomplishes this task.
Testing has been done manually. Test cases to follow but feedback
would be appreciated on
1. The above understanding in case I am missing something
2. The patch itself
Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
---
M src/kudu/tablet/tablet.cc
1 file changed, 25 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/19445/2
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 2
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG@38
PS6, Line 38: Columns - A Non Unique Primary Key column of type INT32
> Ah, I guess the lagging replica is a bit different scenario from what I des
If TS2 is started back as a follower replica, the in-memory auto incrementing counter will be populated to the highest value seen in the WALs which have a corresponding COMMIT message. In this case if this is a new tablet and the WALs are not garbage collected, it would be 5.
If this replica is a follower, the counter will be overwritten by the counter in the raft replicate message of the next INSERT op.
If this is a leader, the next insert OP will have the counter set to 6. The auto incrementing counter would be reset to 0 only if there are no committed insert ops in the WALs (which contain the counter).
Did that clarify the concern?
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 6
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 28 Mar 2023 03:58:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Marton Greber, Tidy Bot, Alexey Serbin, Kudu Jenkins, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19445
to look at the new patch set (#7).
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
KUDU-1945 Update auto incrementing counter during bootstrap
The auto incrementing counter would be reset to zero when the tablet
is being initialized. It is essential to have the correct value of
the counter. There are two cases:
1. WAL segments contain insert operations
In this scenario the WAL segments are replayed and since each insert
operation entry has auto incrementing counter which will be used for
the insert operations present in that entry, as long as we have the
latest insert operation entry in the WAL segments, the auto
incrementing counter is populated correctly during bootstrap.
2. WAL segments do not contain insert operations
In the case, we need to fetch the highest auto incrementing counter
value present in the data which is already flushed and update the
in memory auto incrementing counter appropriately. This patch
accomplishes this task.
There are tests for the bootstrap scenarios where
1. We have no WAL segments with an INSERT OP containing the auto
incrementing column value. We rely on the auto incrementing counter
present in the data directories in this case.
2. We have no WAL segments with auto incrementing column value
and all the rows of the table are deleted. We reset the auto
incrementing counter in this case.
3. We have non committed replicate ops containing INSERT OPs with
the auto incrementing column values.
Manually tested the time taken to populate the auto incrementing
counter:
Columns - A Non Unique Primary Key column of type INT32
- 8 INT64 columns
- 5 STRING columns
Rows - 500k rows with data populated
Time taken in populating the counter during bootstrap:
Min - 235ms
Max - 466ms
Median - 312ms
The total time spent boostrapping the tablet was between 18-25
seconds.
Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
---
M src/kudu/integration-tests/auto_incrementing-itest.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tserver/tablet_server-test-base.h
4 files changed, 321 insertions(+), 39 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/19445/7
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 7
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG@38
PS6, Line 38: Columns - A Non Unique Primary Key column of type INT32
> No nothing changes as we update auto incrementing counter only during boots
Thank you for the clarification.
Just wanted to make sure my understanding is correct. So, a few more questions, please :)
What's going to be the value for the auto-incrementing column once TS2 bootstraps with all INSERT operations GCed, so only the segment with DELETE operations has left? I guess it is going to be 0.
What's the value for the auto-incrementing column for the rest of tablet servers that haven't been restarted? I guess the other two are going to have something non-zero, and that's what left from the very last INSERT.
With a re-election, when the tablet replica at TS2 becomes a leader, INSERTs funneled through that replica at TS2 are going to start with 0 for the auto-incrementing column. Those values will be pushed to other two follower replicas and will override the non-zero value that has been kept there from the prior bunch of INSERT operations.
With that, all the replicas are going to have the same values for the auto-incrementing column for all newly inserted rows, so all is well.
Is my understanding correct?
Thanks!
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 6
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 Mar 2023 05:38:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 7: Code-Review+1
(5 comments)
It seems IWYU isn't yet happy:
>>> Fixing #includes in '/home/jenkins-slave/workspace/kudu-master/0/src/kudu/integration-tests/auto_incrementing-itest.cc'
@@ -18,7 +18,6 @@
// Integration test for flexible partitioning (eg buckets, range partitioning
// of PK subsets, etc).
-#include <cstdint>
#include <memory>
#include <ostream>
#include <string>
@@ -35,23 +34,18 @@
#include "kudu/client/write_op.h"
#include "kudu/common/common.pb.h"
#include "kudu/common/partial_row.h"
-#include "kudu/common/row.h"
#include "kudu/common/schema.h"
#include "kudu/common/wire_protocol.h"
-#include "kudu/common/wire_protocol.pb.h"
#include "kudu/consensus/metadata.pb.h"
#include "kudu/gutil/strings/substitute.h"
#include "kudu/mini-cluster/external_mini_cluster.h"
#include "kudu/rpc/rpc_controller.h"
-#include "kudu/tablet/tablet_replica.h"
+#include "kudu/tablet/tablet.pb.h"
#include "kudu/tserver/tablet_server-test-base.h"
-#include "kudu/tserver/tablet_server.h"
-#include "kudu/tserver/ts_tablet_manager.h"
#include "kudu/tserver/tserver.pb.h"
#include "kudu/util/env_util.h"
#include "kudu/util/monotime.h"
#include "kudu/util/path_util.h"
-#include "kudu/util/slice.h"
#include "kudu/util/status.h"
#include "kudu/util/test_macros.h"
#include "kudu/util/test_util.h"
>>> Fixing #includes in '/home/jenkins-slave/workspace/kudu-master/0/src/kudu/tablet/tablet.h'
@@ -86,7 +86,6 @@
class AlterSchemaOpState;
class CompactionPolicy;
-class DiskRowSet;
class HistoryGcOpts;
class MemRowSet;
class ParticipantOpState;
IWYU would have edited 2 files on your behalf.
http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG@21
PS6, Line 21: the
> this?
missed addressing this one in PS7?
http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG@23
PS6, Line 23: in memory
> nit: in-memory
missing addressing this one in PS7?
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc
File src/kudu/integration-tests/auto_incrementing-itest.cc:
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@394
PS6, Line 394: ASSERT_EQ(results[i].size(), results[i+1].size());
: for (int k = 0; k < results[i].
> That should be the case, the reason being:
It would be great if you could add a comment into the code to explain why this situation is guaranteed to happen.
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@405
PS6, Line 405:
> Incomplete comment?
Yep, but now I forgot what I was going to comment about :)
http://gerrit.cloudera.org:8080/#/c/19445/7/src/kudu/integration-tests/auto_incrementing-itest.cc
File src/kudu/integration-tests/auto_incrementing-itest.cc:
http://gerrit.cloudera.org:8080/#/c/19445/7/src/kudu/integration-tests/auto_incrementing-itest.cc@190
PS7, Line 190: *tablet_uuid = resp.status_and_schema(0).tablet_status().tablet_id();
Do we need to make sure resp.status_and_schema isn't empty? If you don't want to introduce if() clause, adding CHECK() is OK (not great, but at least there wouldn't reading some junk from random location in the memory).
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 7
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 30 Mar 2023 06:05:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 17: Code-Review+2
LGTM
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 17
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Sat, 01 Apr 2023 03:35:11 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Marton Greber, Tidy Bot, Alexey Serbin, Yingchun Lai, Kudu Jenkins, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19445
to look at the new patch set (#15).
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
KUDU-1945 Update auto incrementing counter during bootstrap
The auto incrementing counter would be reset to zero when the tablet
is being initialized. It is essential to have the correct value of
the counter. There are two cases:
1. WAL segments contain insert operations
In this scenario the WAL segments are replayed and since each insert
operation entry has auto incrementing counter which will be used for
the insert operations present in that entry, as long as we have the
latest insert operation entry in the WAL segments, the auto
incrementing counter is populated correctly during bootstrap.
2. WAL segments do not contain insert operations
In this case, we need to fetch the highest auto incrementing counter
value present in the data which is already flushed and update the
in-memory auto incrementing counter appropriately. This patch
accomplishes this task.
There are tests for the bootstrap scenarios where
1. We have no WAL segments with an INSERT OP containing the auto
incrementing column value. We rely on the auto incrementing counter
present in the data directories in this case.
2. We have no WAL segments with auto incrementing column value
and all the rows of the table are deleted. We reset the auto
incrementing counter in this case.
3. We have non committed replicate ops containing INSERT OPs with
the auto incrementing column values.
Manually tested the time taken to populate the auto incrementing
counter:
Columns - A Non Unique Primary Key column of type INT32
- 8 INT64 columns
- 5 STRING columns
Rows - 500k rows with data populated
Time taken in populating the counter during bootstrap:
Min - 235ms
Max - 466ms
Median - 312ms
The total time spent boostrapping the tablet was between 18-25
seconds.
Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
---
M src/kudu/integration-tests/auto_incrementing-itest.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tserver/tablet_server-test-base.h
4 files changed, 335 insertions(+), 42 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/19445/15
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 15
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Marton Greber, Tidy Bot, Alexey Serbin, Yingchun Lai, Kudu Jenkins, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19445
to look at the new patch set (#14).
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
KUDU-1945 Update auto incrementing counter during bootstrap
The auto incrementing counter would be reset to zero when the tablet
is being initialized. It is essential to have the correct value of
the counter. There are two cases:
1. WAL segments contain insert operations
In this scenario the WAL segments are replayed and since each insert
operation entry has auto incrementing counter which will be used for
the insert operations present in that entry, as long as we have the
latest insert operation entry in the WAL segments, the auto
incrementing counter is populated correctly during bootstrap.
2. WAL segments do not contain insert operations
In this case, we need to fetch the highest auto incrementing counter
value present in the data which is already flushed and update the
in-memory auto incrementing counter appropriately. This patch
accomplishes this task.
There are tests for the bootstrap scenarios where
1. We have no WAL segments with an INSERT OP containing the auto
incrementing column value. We rely on the auto incrementing counter
present in the data directories in this case.
2. We have no WAL segments with auto incrementing column value
and all the rows of the table are deleted. We reset the auto
incrementing counter in this case.
3. We have non committed replicate ops containing INSERT OPs with
the auto incrementing column values.
Manually tested the time taken to populate the auto incrementing
counter:
Columns - A Non Unique Primary Key column of type INT32
- 8 INT64 columns
- 5 STRING columns
Rows - 500k rows with data populated
Time taken in populating the counter during bootstrap:
Min - 235ms
Max - 466ms
Median - 312ms
The total time spent boostrapping the tablet was between 18-25
seconds.
Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
---
M src/kudu/integration-tests/auto_incrementing-itest.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tserver/tablet_server-test-base.h
4 files changed, 335 insertions(+), 42 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/19445/14
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 14
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG@38
PS6, Line 38: Columns - A Non Unique Primary Key column of type INT32
> So, what if the first INSERTs are all went into one segment, and only the c
No nothing changes as we update auto incrementing counter only during bootstrapping/decoding of INSERT operations currently.
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 6
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 28 Mar 2023 17:16:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Marton Greber, Tidy Bot, Alexey Serbin, Kudu Jenkins, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19445
to look at the new patch set (#6).
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
KUDU-1945 Update auto incrementing counter during bootstrap
The auto incrementing counter would be reset to zero when the tablet
is being initialized. It is essential to have the correct value of
the counter. There are two cases:
1. WAL segments contain insert operations
In this scenario the WAL segments are replayed and since each insert
operation entry has auto incrementing counter which will be used for
the insert operations present in that entry, as long as we have the
latest insert operation entry in the WAL segments, the auto
incrementing counter is populated correctly during bootstrap.
2. WAL segments do not contain insert operations
In the case, we need to fetch the highest auto incrementing counter
value present in the data which is already flushed and update the
in memory auto incrementing counter appropriately. This patch
accomplishes this task.
There are tests for the bootstrap scenarios where
1. We have no WAL segments with an INSERT OP containing the auto
incrementing column value. We rely on the auto incrementing counter
present in the data directories in this case.
2. We have no WAL segments with auto incrementing column value
and all the rows of the table are deleted. We reset the auto
incrementing counter in this case.
3. We have non committed replicate ops containing INSERT OPs with
the auto incrementing column values.
Manually tested the time taken to populate the auto incrementing
counter:
Columns - A Non Unique Primary Key column of type INT32
- 8 INT64 columns
- 5 STRING columns
Rows - 500k rows with data populated
Time taken in populating the counter during bootstrap:
Min - 235ms
Max - 466ms
Median - 312ms
The total time spent boostrapping the tablet was between 18-25
seconds.
Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
---
M src/kudu/integration-tests/auto_incrementing-itest.cc
M src/kudu/tablet/tablet.cc
2 files changed, 327 insertions(+), 34 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/19445/6
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 6
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 16:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/19445/14/src/kudu/integration-tests/auto_incrementing-itest.cc
File src/kudu/integration-tests/auto_incrementing-itest.cc:
http://gerrit.cloudera.org:8080/#/c/19445/14/src/kudu/integration-tests/auto_incrementing-itest.cc@306
PS14, Line 306: // There could still be a WAL entry with a DELETE OP.
: // This might cause the auto incrementing counter to be not set to 0 unless
: // FlushDeltaMRS is called. We restart servers to force elections which in turn
: // writes more WAL entries which causes these flushes.
> Yes, but since there would be only one leader at a given point of time, the
Ah, OK -- for some reason I thought the check verification below was comparing the rows from different tablet servers.
On a separate note, I recall that presence of DELETES in the WAL doesn't matter -- there isn't a value for the auto-incrementing column in a DELETE op, right? Why to mention the presence of WAL entries with a delete operation then?
http://gerrit.cloudera.org:8080/#/c/19445/16/src/kudu/integration-tests/auto_incrementing-itest.cc
File src/kudu/integration-tests/auto_incrementing-itest.cc:
http://gerrit.cloudera.org:8080/#/c/19445/16/src/kudu/integration-tests/auto_incrementing-itest.cc@294
PS16, Line 294: // We are still yet to GC the wal segments or flush the deletes,
: // check back the server after 50ms.
nit: a misaligned comment
http://gerrit.cloudera.org:8080/#/c/19445/16/src/kudu/integration-tests/auto_incrementing-itest.cc@310
PS16, Line 310: for (int i = 0; i < kNumTabletServers * 3; i++) {
Instead, why not just shutdown all tablet servers, and then restart all of them?
http://gerrit.cloudera.org:8080/#/c/19445/16/src/kudu/integration-tests/auto_incrementing-itest.cc@311
PS16, Line 311: 3
If doing this fancy stuff, this should be kNumTabletServers, not 3, right?
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 16
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Sat, 01 Apr 2023 02:42:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 12:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/19445/12/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/19445/12/src/kudu/tablet/tablet.cc@442
PS12, Line 442: "
nit: How about output the failed reason, i.e. s.ToString().
http://gerrit.cloudera.org:8080/#/c/19445/12/src/kudu/tablet/tablet.cc@496
PS12, Line 496: Stopwatch sw(Stopwatch::THIS_THREAD);
How about using LOG_TIMING* macros for this to simplify code?
http://gerrit.cloudera.org:8080/#/c/19445/12/src/kudu/tablet/tablet.cc@518
PS12, Line 518: if (counter > auto_incrementing_counter_) {
Just curious, when counter <= auto_incrementing_counter_ ?
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 12
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Fri, 31 Mar 2023 16:35:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19445/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19445/1//COMMIT_MSG@28
PS1, Line 28: 2. The patch itself
> What's impact for bootstrap of a tablet with large data set?
This will slow down the bootstrap process due to additional processing. The exact metrics need to be tested but I wouldn't expect a significant increase as the data is already present in the memory which is already read from the disk and we are iterating on a single column.
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 1
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Sat, 28 Jan 2023 05:15:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 8: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 8
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 30 Mar 2023 20:27:54 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Marton Greber, Tidy Bot, Alexey Serbin, Kudu Jenkins, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19445
to look at the new patch set (#4).
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
KUDU-1945 Update auto incrementing counter during bootstrap
The auto incrementing counter would be reset to zero when the tablet
is being initialized. It is essential to have the correct value of
the counter. There are two cases:
1. WAL segments contain insert operations
In this scenario the WAL segments are replayed and since each insert
operation entry has auto incrementing counter which will be used for
the insert operations present in that entry, as long as we have the
latest insert operation entry in the WAL segments, the auto
incrementing counter is populated correctly during bootstrap.
2. WAL segments do not contain insert operations
In the case, we need to fetch the highest auto incrementing counter
value present in the data which is already flushed and update the
in memory auto incrementing counter appropriately. This patch
accomplishes this task.
Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
---
M src/kudu/integration-tests/auto_incrementing-itest.cc
M src/kudu/tablet/tablet.cc
2 files changed, 315 insertions(+), 34 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/19445/4
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 4
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] [WIP]KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: [WIP]KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 2:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/19445/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19445/2//COMMIT_MSG@14
PS2, Line 14: In this scenario the WAL segments are replayed and since each insert
: operation entry has auto incrementing counter which will be used for
: the insert operations present in that entry, as long as we have the
: latest insert operation entry in the WAL segments, the auto
: incrementing counter is populated correctly during bootstrap.
Just wanted to make sure: is that working as expected when WAL contains non-replicated ops that are to be discarded because the majority of replicas eventually converged on different history of operations after reconciling of the WAL?
http://gerrit.cloudera.org:8080/#/c/19445/2//COMMIT_MSG@20
PS2, Line 20: 2. WAL segments do not contain insert operations
: In the case, we need to fetch the highest auto incrementing counter
: value present in the data which is already flushed and update the
: in memory auto incrementing counter appropriately. This patch
: accomplishes this task.
What if current snapshot of the data in the tablet doesn't have a single row? Is it safe to assume we can start the auto-incrementing counter with 0 in that case?
http://gerrit.cloudera.org:8080/#/c/19445/2//COMMIT_MSG@25
PS2, Line 25: Testing has been done manually
It would be great to add test coverage to make sure this works as expected in both cases. Covering the case of a tablet that has been copied over from one tablet server to another could be a great addition as well.
Automating these testing is a good idea since it would allow to catch regressions, if any. Otherwise, I don't think anybody has a capacity to test this manually every Kudu release.
http://gerrit.cloudera.org:8080/#/c/19445/2/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/19445/2/src/kudu/tablet/tablet.cc@433
PS2, Line 433: schema()->has_auto_incrementing()
Would be great to add a comment why this is safe to look just at the current schema version for the presence of the auto-increment column.
http://gerrit.cloudera.org:8080/#/c/19445/2/src/kudu/tablet/tablet.cc@440
PS2, Line 440: 32 * 1024
Could you add a comment to explain why 32K is used here for the row block memory size?
http://gerrit.cloudera.org:8080/#/c/19445/2/src/kudu/tablet/tablet.cc@449
PS2, Line 449:
nit: drop the space ?
http://gerrit.cloudera.org:8080/#/c/19445/2/src/kudu/tablet/tablet.cc@453
PS2, Line 453: LOG(INFO) << "Setting AIC from data to: "<< counter;
I think this might look confusing. First, there might be be multiple diskrowsets for a tablet. Second, there isn't information on the tablet ID in the output.
With that, why to output this at all?
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 2
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 Mar 2023 20:14:34 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG@38
PS6, Line 38: Columns - A Non Unique Primary Key column of type INT32
> What if we have just one replica that lags behind two others, where the mis
Ah, I guess the lagging replica is a bit different scenario from what I described in the beginning of the comment, but let's focus on the detailed scenario first, please.
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 6
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 28 Mar 2023 02:08:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 6:
(36 comments)
http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG@21
PS6, Line 21: the
this?
http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG@23
PS6, Line 23: in memory
nit: in-memory
http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG@35
PS6, Line 35:
I guess there is also a case when those non-commited operations are DELETEs.
I guess reading those ops to retrieve highest available value for the auto-incrementing column happens after reconciliation of Raft logs, so all the replicas has common history in the result WAL.
http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG@38
PS6, Line 38: Columns - A Non Unique Primary Key column of type INT32
What if we have just one replica that lags behind two others, where the missing operations are DELETEs?
Say, what about the following scenario:
1. All three tablet replicas are up and running.
2. 5 INSERTS arrive.
3. 5 DELETES arrive (so, the tablet has no rows).
4. Tablet server TS2 is shut down.
5. The tablet server TS2 is started back, and after tablet bootstratpping it hosts a follower replica, not a leader replica.
6. 5 more INSERTS arrive.
Will the replica at TS2 have the proper value for the auto-incrementing column? What if re-election happens and the replica at TS2 becomes a new leader when new INSERTs arrive?
Thanks!
http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG@49
PS6, Line 49: Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
nit: keep an empty line between the commit description and 'Change-Id' line for better readbility
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc
File src/kudu/integration-tests/auto_incrementing-itest.cc:
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@71
PS6, Line 71: using kudu::tablet::TabletReplica;
> warning: using decl 'TabletReplica' is unused [misc-unused-using-decls]
consider remove this 'using'?
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@132
PS6, Line 132: void DeleteRow
Why not to make this method returning Status and use RETURN_NOT_OK() instead of CHECK_OK()? If necessary, you could wrap this DeleteRow() into CHECK_OK() at the call site.
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@161
PS6, Line 161: // Stringify Rows from the response.
Is it possible to re-use TabletServerTestBase::StringifyRowsFromResponse()? You could make it static and try to re-use here?
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@168
PS6, Line 168:
nit: fix the indent?
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@172
PS6, Line 172:
nit: fix the indent?
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@206
PS6, Line 206: DCHECK_EQ
ASSERT_EQ?
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@219
PS6, Line 219: opts.extra_tserver_flags.emplace_back("--log_segment_size_bytes_for_tests=1024");
: opts.extra_tserver_flags.emplace_back("--log_max_segments_to_retain=2");
: opts.extra_tserver_flags.emplace_back("--maintenance_manager_polling_interval_ms=10");
: opts.extra_tserver_flags.emplace_back("--maintenance_manager_num_threads=4");
: opts.extra_tserver_flags.emplace_back("--flush_threshold_secs=1");
: opts.extra_tserver_flags.emplace_back("--log_compression_codec=no_compression");
nit: use initializers list for opts.extra_tserver_flags instead?
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@238
PS6, Line 238: cluster_->tserver_proxy(0)->ListTablets(req, &resp, &rpc);
Wrap this into ASSERT_OK()?
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@239
PS6, Line 239: DCHECK_EQ(1, resp.status_and_schema_size());
Why not to use ASSERT_EQ() here instead?
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@240
PS6, Line 240: string tablet_uuid
nit: could be 'const string& tablet_uuid'?
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@245
PS6, Line 245: +
nit: separate '+' from the arguments with spaces
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@267
PS6, Line 267: cluster_->Restart()
Wrap Restart() into ASSERT_OK()?
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@284
PS6, Line 284: TEST_F(AutoIncrementingItest, BootstrapNoWalsNoData) {
Is it possible to separate the majority of the code in this BootstrapNoWalsNoData and prior BootstrapWithNoWals scenario into a method/function to avoid duplicating the code?
Alternatively, you could consider adding a new parameterized test based on AutoIncrementingItest.
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@287
PS6, Line 287: opts.extra_tserver_flags.emplace_back("--log_segment_size_bytes_for_tests=1024");
: opts.extra_tserver_flags.emplace_back("--log_max_segments_to_retain=2");
: opts.extra_tserver_flags.emplace_back("--maintenance_manager_polling_interval_ms=10");
: opts.extra_tserver_flags.emplace_back("--maintenance_manager_num_threads=4");
: opts.extra_tserver_flags.emplace_back("--flush_threshold_secs=1");
: opts.extra_tserver_flags.emplace_back("--log_compression_codec=no_compression");
nit: use initializers list for opts.extra_tserver_flags instead?
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@306
PS6, Line 306: cluster_->tserver_proxy(0)->ListTablets(req, &resp, &rpc);
Wrap this into ASSERT_OK()?
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@307
PS6, Line 307: DCHECK_EQ
Why not to use ASSERT_EQ() instead?
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@334
PS6, Line 334: cluster_->Restart();
Wrap this into ASSERT_OK()?
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@380
PS6, Line 380: raft
nit: Raft
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@381
PS6, Line 381: InsertData
Wrap into CHECK_OK()? Alternatively, store the status per thread and check for failures upon threads' completion.
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@381
PS6, Line 381: 5);
Why to sleep between each row, at all? Would be great to comment on the reasoning behind.
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@383
PS6, Line 383: // Ensure at least a couple of rows are attempted to be
: // written before shutting down.
: SleepFor(MonoDelta::FromMilliseconds(10));
: // Shutdown the tablet servers hosting followers while the data is still being written.
: for (int i = 0; i < kNumTabletServers; i++) {
: if (i != leader_ts_index) {
: cluster_->tablet_server(i)->Shutdown();
: }
: }
Would it be easier to run these in their own threads, but run the writer in the main test thread? Shutdown() doesn't return a status, so need to check out on the results at least given the signature of the method?
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@394
PS6, Line 394: leader replica
nit: the leader replica
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@394
PS6, Line 394: // At this point tablet server hosting leader replica would be having a replicate op without
: // a corresponding COMMIT message
Is there a way to ensure that's exactly the case? Maybe, use smallest possible WAL segments and count their number at different tablet servers?
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@400
PS6, Line 400: CHECK_OK
Why not to use ASSERT_OK() here?
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@405
PS6, Line 405: ASSERT_OK(InsertData(kNumRows, kNumRows * 2));
Is there a way to check for
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@410
PS6, Line 410: CHECK_OK
Why not to use ASSERT_OK() here?
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@411
PS6, Line 411: leader_ts_index + kNumTabletServers + 1
Could this part be shorten to leader_ts_index + 1?
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@421
PS6, Line 421: ASSERT_EQ(results[i].size(), results[i+1].size());
: for (int k = 0; k < results[i].size(); k++) {
: ASSERT_EQ(results[i][k], results[i+1][k]);
: }
Does it make sense to verify this across all the tablet replicas, not just the first two?
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/tablet/tablet.cc@437
PS6, Line 437: if (schema()->has_auto_incrementing()) {
Consider moving this code out into a separate method.
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/tablet/tablet.cc@442
PS6, Line 442: TODO(achennaka): Materialize only
That's good point! Is it possible to do so in this patch or you are planning to post a follow-up one?
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/tablet/tablet.cc@466
PS6, Line 466: LOG_WITH_PREFIX(INFO) << "Fetching the auto incrementing counter took "
: << delta.ToMilliseconds() << "ms";
It's better to use Stopwatch(Stopwatch::THIS_THREAD) for this purpose -- it provides more information on the system, user, and real time.
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 6
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 28 Mar 2023 01:45:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 14: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 14
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Fri, 31 Mar 2023 19:17:34 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Marton Greber, Tidy Bot, Alexey Serbin, Kudu Jenkins, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19445
to look at the new patch set (#9).
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
KUDU-1945 Update auto incrementing counter during bootstrap
The auto incrementing counter would be reset to zero when the tablet
is being initialized. It is essential to have the correct value of
the counter. There are two cases:
1. WAL segments contain insert operations
In this scenario the WAL segments are replayed and since each insert
operation entry has auto incrementing counter which will be used for
the insert operations present in that entry, as long as we have the
latest insert operation entry in the WAL segments, the auto
incrementing counter is populated correctly during bootstrap.
2. WAL segments do not contain insert operations
In this case, we need to fetch the highest auto incrementing counter
value present in the data which is already flushed and update the
in-memory auto incrementing counter appropriately. This patch
accomplishes this task.
There are tests for the bootstrap scenarios where
1. We have no WAL segments with an INSERT OP containing the auto
incrementing column value. We rely on the auto incrementing counter
present in the data directories in this case.
2. We have no WAL segments with auto incrementing column value
and all the rows of the table are deleted. We reset the auto
incrementing counter in this case.
3. We have non committed replicate ops containing INSERT OPs with
the auto incrementing column values.
Manually tested the time taken to populate the auto incrementing
counter:
Columns - A Non Unique Primary Key column of type INT32
- 8 INT64 columns
- 5 STRING columns
Rows - 500k rows with data populated
Time taken in populating the counter during bootstrap:
Min - 235ms
Max - 466ms
Median - 312ms
The total time spent boostrapping the tablet was between 18-25
seconds.
Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
---
M src/kudu/integration-tests/auto_incrementing-itest.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tserver/tablet_server-test-base.h
4 files changed, 324 insertions(+), 42 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/19445/9
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 9
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 5:
(5 comments)
Looks good. Only a few nits.
http://gerrit.cloudera.org:8080/#/c/19445/5/src/kudu/integration-tests/auto_incrementing-itest.cc
File src/kudu/integration-tests/auto_incrementing-itest.cc:
http://gerrit.cloudera.org:8080/#/c/19445/5/src/kudu/integration-tests/auto_incrementing-itest.cc@338
PS5, Line 338: *
nit: space around *
http://gerrit.cloudera.org:8080/#/c/19445/5/src/kudu/integration-tests/auto_incrementing-itest.cc@415
PS5, Line 415: int flag = 3; flag > 0; flag--
nit: int i = 0; i < 3; i++
http://gerrit.cloudera.org:8080/#/c/19445/5/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/19445/5/src/kudu/tablet/tablet.cc@446
PS5, Line 446: 32K should be a good start
you remove the initial size 32K for mem
http://gerrit.cloudera.org:8080/#/c/19445/5/src/kudu/tablet/tablet.cc@466
PS5, Line 466: AIC
AIC is not common abbreviation, not easy to understand.
http://gerrit.cloudera.org:8080/#/c/19445/5/src/kudu/tablet/tablet.cc@466
PS5, Line 466: <<
add a space before <<
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 5
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 23 Mar 2023 00:21:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Marton Greber, Tidy Bot, Alexey Serbin, Kudu Jenkins, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19445
to look at the new patch set (#11).
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
KUDU-1945 Update auto incrementing counter during bootstrap
The auto incrementing counter would be reset to zero when the tablet
is being initialized. It is essential to have the correct value of
the counter. There are two cases:
1. WAL segments contain insert operations
In this scenario the WAL segments are replayed and since each insert
operation entry has auto incrementing counter which will be used for
the insert operations present in that entry, as long as we have the
latest insert operation entry in the WAL segments, the auto
incrementing counter is populated correctly during bootstrap.
2. WAL segments do not contain insert operations
In this case, we need to fetch the highest auto incrementing counter
value present in the data which is already flushed and update the
in-memory auto incrementing counter appropriately. This patch
accomplishes this task.
There are tests for the bootstrap scenarios where
1. We have no WAL segments with an INSERT OP containing the auto
incrementing column value. We rely on the auto incrementing counter
present in the data directories in this case.
2. We have no WAL segments with auto incrementing column value
and all the rows of the table are deleted. We reset the auto
incrementing counter in this case.
3. We have non committed replicate ops containing INSERT OPs with
the auto incrementing column values.
Manually tested the time taken to populate the auto incrementing
counter:
Columns - A Non Unique Primary Key column of type INT32
- 8 INT64 columns
- 5 STRING columns
Rows - 500k rows with data populated
Time taken in populating the counter during bootstrap:
Min - 235ms
Max - 466ms
Median - 312ms
The total time spent boostrapping the tablet was between 18-25
seconds.
Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
---
M src/kudu/integration-tests/auto_incrementing-itest.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tserver/tablet_server-test-base.h
4 files changed, 328 insertions(+), 42 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/19445/11
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 11
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Marton Greber, Tidy Bot, Alexey Serbin, Yingchun Lai, Kudu Jenkins, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19445
to look at the new patch set (#16).
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
KUDU-1945 Update auto incrementing counter during bootstrap
The auto incrementing counter would be reset to zero when the tablet
is being initialized. It is essential to have the correct value of
the counter. There are two cases:
1. WAL segments contain insert operations
In this scenario the WAL segments are replayed and since each insert
operation entry has auto incrementing counter which will be used for
the insert operations present in that entry, as long as we have the
latest insert operation entry in the WAL segments, the auto
incrementing counter is populated correctly during bootstrap.
2. WAL segments do not contain insert operations
In this case, we need to fetch the highest auto incrementing counter
value present in the data which is already flushed and update the
in-memory auto incrementing counter appropriately. This patch
accomplishes this task.
There are tests for the bootstrap scenarios where
1. We have no WAL segments with an INSERT OP containing the auto
incrementing column value. We rely on the auto incrementing counter
present in the data directories in this case.
2. We have no WAL segments with auto incrementing column value
and all the rows of the table are deleted. We reset the auto
incrementing counter in this case.
3. We have non committed replicate ops containing INSERT OPs with
the auto incrementing column values.
Manually tested the time taken to populate the auto incrementing
counter:
Columns - A Non Unique Primary Key column of type INT32
- 8 INT64 columns
- 5 STRING columns
Rows - 500k rows with data populated
Time taken in populating the counter during bootstrap:
Min - 235ms
Max - 466ms
Median - 312ms
The total time spent boostrapping the tablet was between 18-25
seconds.
Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
---
M src/kudu/integration-tests/auto_incrementing-itest.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tserver/tablet_server-test-base.h
4 files changed, 335 insertions(+), 42 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/19445/16
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 16
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 8:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG@21
PS6, Line 21: thi
> this?
Done
http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG@21
PS6, Line 21: thi
> missed addressing this one in PS7?
Done
http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG@23
PS6, Line 23: in-memory
> nit: in-memory
Done
http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG@23
PS6, Line 23: in-memory
> missing addressing this one in PS7?
Done
http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG@49
PS6, Line 49:
> nit: keep an empty line between the commit description and 'Change-Id' line
Done
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc
File src/kudu/integration-tests/auto_incrementing-itest.cc:
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@394
PS6, Line 394: ASSERT_EQ(results[i][k], results[i + 1][k]);
: }
> It would be great if you could add a comment into the code to explain why t
Done
http://gerrit.cloudera.org:8080/#/c/19445/7/src/kudu/integration-tests/auto_incrementing-itest.cc
File src/kudu/integration-tests/auto_incrementing-itest.cc:
http://gerrit.cloudera.org:8080/#/c/19445/7/src/kudu/integration-tests/auto_incrementing-itest.cc@190
PS7, Line 190: unique_ptr<cluster::ExternalMiniCluster> cluster_;
> Do we need to make sure resp.status_and_schema isn't empty? If you don't w
Done
http://gerrit.cloudera.org:8080/#/c/19445/7/src/kudu/integration-tests/auto_incrementing-itest.cc@281
PS7, Line 281:
> nit: space around +
Done
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 8
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 30 Mar 2023 20:01:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19445/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19445/1//COMMIT_MSG@28
PS1, Line 28: 2. The patch itself
What's impact for bootstrap of a tablet with large data set?
http://gerrit.cloudera.org:8080/#/c/19445/1/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/19445/1/src/kudu/tablet/tablet.cc@433
PS1, Line 433: if (schema()->has_auto_incrementing()) {
please add a comment here
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 1
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Jan 2023 22:26:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 4:
(16 comments)
http://gerrit.cloudera.org:8080/#/c/19445/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19445/3//COMMIT_MSG@24
PS3, Line 24: accomplishes this task.
add what are tests added in this patch
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/integration-tests/auto_incrementing-itest.cc
File src/kudu/integration-tests/auto_incrementing-itest.cc:
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/integration-tests/auto_incrementing-itest.cc@28
PS3, Line 28: #include <utility>
: #include <vector>
why use <kudu/client/write_op.h> instead of "kudu/client/write_op.h"? Should we keep alphabet order?
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/integration-tests/auto_incrementing-itest.cc@111
PS3, Line 111: lower.
nit: created
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/integration-tests/auto_incrementing-itest.cc@127
PS3, Line 127:
In InsertData(), we open table before creating new session. But here we create new session before opening table. Should we keep the order consistently?
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/integration-tests/auto_incrementing-itest.cc@174
PS3, Line 174: resul
nit: indent one less space
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/integration-tests/auto_incrementing-itest.cc@230
PS3, Line 230:
nit: at least
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/integration-tests/auto_incrementing-itest.cc@247
PS3, Line 247: nsu
Since loop variable is changed in body, it's better to use a while loop.
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/integration-tests/auto_incrementing-itest.cc@270
PS3, Line 270: z
nit: Upper case D?
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/integration-tests/auto_incrementing-itest.cc@308
PS3, Line 308: lete
use while loop
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/integration-tests/auto_incrementing-itest.cc@349
PS3, Line 349: SERT_OK(clu
Why keeps to update table_uuid in the loop?
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/integration-tests/auto_incrementing-itest.cc@351
PS3, Line 351: // Create a table.
break loop if the leader is found
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/integration-tests/auto_incrementing-itest.cc@361
PS3, Line 361: ->tserv
nit: at least
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/integration-tests/auto_incrementing-itest.cc@373
PS3, Line 373:
nit: kNumTabletServers - 1
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/integration-tests/auto_incrementing-itest.cc@385
PS3, Line 385:
nit: have same data
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/integration-tests/auto_incrementing-itest.cc@390
PS3, Line 390: for (int i = 0; i <
use for loop
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/tablet/tablet.cc@451
PS3, Line 451: i
nit: remove space
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 4
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Mar 2023 08:19:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 4:
(16 comments)
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/integration-tests/auto_incrementing-itest.cc
File src/kudu/integration-tests/auto_incrementing-itest.cc:
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/integration-tests/auto_incrementing-itest.cc@28
PS3, Line 28: #include <utility>
: #include <vector>
> why use <kudu/client/write_op.h> instead of "kudu/client/write_op.h"? Shoul
This is taken care of in PS4
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/integration-tests/auto_incrementing-itest.cc@111
PS3, Line 111: lower.
> nit: created
Done
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/integration-tests/auto_incrementing-itest.cc@127
PS3, Line 127:
> In InsertData(), we open table before creating new session. But here we cre
Done
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/integration-tests/auto_incrementing-itest.cc@157
PS3, Line 157: },2);
> warning: multiple declarations in a single statement reduces readability [r
Done
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/integration-tests/auto_incrementing-itest.cc@174
PS3, Line 174: resul
> nit: indent one less space
Done
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/integration-tests/auto_incrementing-itest.cc@230
PS3, Line 230:
> nit: at least
Done
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/integration-tests/auto_incrementing-itest.cc@247
PS3, Line 247: nsu
> Since loop variable is changed in body, it's better to use a while loop.
Done
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/integration-tests/auto_incrementing-itest.cc@270
PS3, Line 270: z
> nit: Upper case D?
Done
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/integration-tests/auto_incrementing-itest.cc@308
PS3, Line 308: lete
> use while loop
Done
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/integration-tests/auto_incrementing-itest.cc@349
PS3, Line 349: SERT_OK(clu
> Why keeps to update table_uuid in the loop?
Done
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/integration-tests/auto_incrementing-itest.cc@351
PS3, Line 351: // Create a table.
> break loop if the leader is found
Done
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/integration-tests/auto_incrementing-itest.cc@361
PS3, Line 361: ->tserv
> nit: at least
Done
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/integration-tests/auto_incrementing-itest.cc@373
PS3, Line 373:
> nit: kNumTabletServers - 1
Done
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/integration-tests/auto_incrementing-itest.cc@385
PS3, Line 385:
> nit: have same data
Done
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/integration-tests/auto_incrementing-itest.cc@390
PS3, Line 390: for (int i = 0; i <
> use for loop
Done
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/19445/3/src/kudu/tablet/tablet.cc@451
PS3, Line 451: i
> nit: remove space
Done
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 4
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Mar 2023 23:07:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Marton Greber, Tidy Bot, Alexey Serbin, Kudu Jenkins, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19445
to look at the new patch set (#5).
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
KUDU-1945 Update auto incrementing counter during bootstrap
The auto incrementing counter would be reset to zero when the tablet
is being initialized. It is essential to have the correct value of
the counter. There are two cases:
1. WAL segments contain insert operations
In this scenario the WAL segments are replayed and since each insert
operation entry has auto incrementing counter which will be used for
the insert operations present in that entry, as long as we have the
latest insert operation entry in the WAL segments, the auto
incrementing counter is populated correctly during bootstrap.
2. WAL segments do not contain insert operations
In the case, we need to fetch the highest auto incrementing counter
value present in the data which is already flushed and update the
in memory auto incrementing counter appropriately. This patch
accomplishes this task.
There are tests for the bootstrap scenarios where
1. We have no WAL segments with an INSERT OP containing the auto
incrementing column value. We rely on the auto incrementing counter
present in the data directories in this case.
2. We have no WAL segments with auto incrementing column value
and all the rows of the table are deleted. We reset the auto
incrementing counter in this case.
3. We have non committed replicate ops containing INSERT OPs with
the auto incrementing column values.
Manually tested the time taken to populate the auto incrementing
counter:
Columns - A Non Unique Primary Key column of type INT32
- 8 INT64 columns
- 5 STRING columns
Rows - 500k rows with data populated
Time taken in populating the counter during bootstrap:
Min - 235ms
Max - 466ms
Median - 312ms
The total time spent boostrapping the tablet was between 18-25
seconds.
Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
---
M src/kudu/integration-tests/auto_incrementing-itest.cc
M src/kudu/tablet/tablet.cc
2 files changed, 328 insertions(+), 34 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/19445/5
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 5
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 6: Code-Review+1
(3 comments)
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc
File src/kudu/integration-tests/auto_incrementing-itest.cc:
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@341
PS6, Line 341:
nit: extra space
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@419
PS6, Line 419: 3
kNumTabletServers
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@420
PS6, Line 420: 2
kNumTabletServers - 1
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 6
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 23 Mar 2023 18:54:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19445/7/src/kudu/integration-tests/auto_incrementing-itest.cc
File src/kudu/integration-tests/auto_incrementing-itest.cc:
http://gerrit.cloudera.org:8080/#/c/19445/7/src/kudu/integration-tests/auto_incrementing-itest.cc@281
PS7, Line 281: +
nit: space around +
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 7
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 30 Mar 2023 06:34:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 11:
> Build Started http://jenkins.kudu.apache.org/job/kudu-gerrit/27449/
It seems the new test is still failing in TSAN builds: http://dist-test.cloudera.org/job?job_id=jenkins-slave.1680241071.1632174
/home/jenkins-slave/workspace/kudu-master/3/src/kudu/integration-tests/auto_incrementing-itest.cc:314: Failure
Expected equality of these values: Substitute("(int32 c0=$0, int64 $1=$2)", i + kNumRows, Schema::GetAutoIncrementingColumnName(), i + 1)
Which is: "(int32 c0=200, int64 auto_incrementing_id=1)"
results[i] Which is: "(int32 c0=200, int64 auto_incrementing_id=201)"
[ FAILED ] AutoIncrementingItest.BootstrapNoWalsNoData
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 11
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 31 Mar 2023 06:05:20 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
KUDU-1945 Update auto incrementing counter during bootstrap
The auto incrementing counter would be reset to zero when the tablet
is being initialized. It is essential to have the correct value of
the counter. There are two cases:
1. WAL segments contain insert operations
In this scenario the WAL segments are replayed and since each insert
operation entry has auto incrementing counter which will be used for
the insert operations present in that entry, as long as we have the
latest insert operation entry in the WAL segments, the auto
incrementing counter is populated correctly during bootstrap.
2. WAL segments do not contain insert operations
In this case, we need to fetch the highest auto incrementing counter
value present in the data which is already flushed and update the
in-memory auto incrementing counter appropriately. This patch
accomplishes this task.
There are tests for the bootstrap scenarios where
1. We have no WAL segments with an INSERT OP containing the auto
incrementing column value. We rely on the auto incrementing counter
present in the data directories in this case.
2. We have no WAL segments with auto incrementing column value
and all the rows of the table are deleted. We reset the auto
incrementing counter in this case.
3. We have non committed replicate ops containing INSERT OPs with
the auto incrementing column values.
Manually tested the time taken to populate the auto incrementing
counter:
Columns - A Non Unique Primary Key column of type INT32
- 8 INT64 columns
- 5 STRING columns
Rows - 500k rows with data populated
Time taken in populating the counter during bootstrap:
Min - 235ms
Max - 466ms
Median - 312ms
The total time spent boostrapping the tablet was between 18-25
seconds.
Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Reviewed-on: http://gerrit.cloudera.org:8080/19445
Reviewed-by: Alexey Serbin <al...@apache.org>
Tested-by: Kudu Jenkins
---
M src/kudu/integration-tests/auto_incrementing-itest.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tserver/tablet_server-test-base.h
4 files changed, 336 insertions(+), 42 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 18
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 6:
(36 comments)
http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG@35
PS6, Line 35:
> I guess there is also a case when those non-commited operations are DELETEs
Reading/Decoding DELETE ops will not affect the auto incrementing counter. It is only updated when reading INSERT ops.
http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG@38
PS6, Line 38: Columns - A Non Unique Primary Key column of type INT32
> Thank you for the clarification.
Yes that is correct. At a given point the in-memory counter values might be different for different replicas but since the followers update the value based on what the leader raft replicates with an INSERT op, the column value is going to be the same.
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc
File src/kudu/integration-tests/auto_incrementing-itest.cc:
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@71
PS6, Line 71: using kudu::tablet::TabletReplica;
> consider remove this 'using'?
Done
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@132
PS6, Line 132: void DeleteRow
> Why not to make this method returning Status and use RETURN_NOT_OK() instea
Done
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@161
PS6, Line 161: // Stringify Rows from the response.
> Is it possible to re-use TabletServerTestBase::StringifyRowsFromResponse()
Done
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@168
PS6, Line 168:
> nit: fix the indent?
Done
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@172
PS6, Line 172:
> nit: fix the indent?
Done
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@206
PS6, Line 206: DCHECK_EQ
> ASSERT_EQ?
Done
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@219
PS6, Line 219: opts.extra_tserver_flags.emplace_back("--log_segment_size_bytes_for_tests=1024");
: opts.extra_tserver_flags.emplace_back("--log_max_segments_to_retain=2");
: opts.extra_tserver_flags.emplace_back("--maintenance_manager_polling_interval_ms=10");
: opts.extra_tserver_flags.emplace_back("--maintenance_manager_num_threads=4");
: opts.extra_tserver_flags.emplace_back("--flush_threshold_secs=1");
: opts.extra_tserver_flags.emplace_back("--log_compression_codec=no_compression");
> nit: use initializers list for opts.extra_tserver_flags instead?
Done
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@238
PS6, Line 238: cluster_->tserver_proxy(0)->ListTablets(req, &resp, &rpc);
> Wrap this into ASSERT_OK()?
Done
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@239
PS6, Line 239: DCHECK_EQ(1, resp.status_and_schema_size());
> Why not to use ASSERT_EQ() here instead?
Done
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@240
PS6, Line 240: string tablet_uuid
> nit: could be 'const string& tablet_uuid'?
Done
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@245
PS6, Line 245: +
> nit: separate '+' from the arguments with spaces
Done
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@267
PS6, Line 267: cluster_->Restart()
> Wrap Restart() into ASSERT_OK()?
Done
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@284
PS6, Line 284: TEST_F(AutoIncrementingItest, BootstrapNoWalsNoData) {
> Is it possible to separate the majority of the code in this BootstrapNoWals
Done
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@287
PS6, Line 287: opts.extra_tserver_flags.emplace_back("--log_segment_size_bytes_for_tests=1024");
: opts.extra_tserver_flags.emplace_back("--log_max_segments_to_retain=2");
: opts.extra_tserver_flags.emplace_back("--maintenance_manager_polling_interval_ms=10");
: opts.extra_tserver_flags.emplace_back("--maintenance_manager_num_threads=4");
: opts.extra_tserver_flags.emplace_back("--flush_threshold_secs=1");
: opts.extra_tserver_flags.emplace_back("--log_compression_codec=no_compression");
> nit: use initializers list for opts.extra_tserver_flags instead?
Done
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@306
PS6, Line 306: cluster_->tserver_proxy(0)->ListTablets(req, &resp, &rpc);
> Wrap this into ASSERT_OK()?
Done
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@307
PS6, Line 307: DCHECK_EQ
> Why not to use ASSERT_EQ() instead?
Done
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@334
PS6, Line 334: cluster_->Restart();
> Wrap this into ASSERT_OK()?
Done
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@341
PS6, Line 341:
> nit: extra space
Done
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@380
PS6, Line 380: raft
> nit: Raft
Done
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@381
PS6, Line 381: InsertData
> Wrap into CHECK_OK()? Alternatively, store the status per thread and check
This is now a part of the main thread and status will not be ok() as the inserts would fail due to servers being shutdown
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@381
PS6, Line 381: 5);
> Why to sleep between each row, at all? Would be great to comment on the re
Done
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@383
PS6, Line 383: // Ensure at least a couple of rows are attempted to be
: // written before shutting down.
: SleepFor(MonoDelta::FromMilliseconds(10));
: // Shutdown the tablet servers hosting followers while the data is still being written.
: for (int i = 0; i < kNumTabletServers; i++) {
: if (i != leader_ts_index) {
: cluster_->tablet_server(i)->Shutdown();
: }
: }
> Would it be easier to run these in their own threads, but run the writer in
Done
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@394
PS6, Line 394: leader replica
> nit: the leader replica
Done
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@394
PS6, Line 394: // At this point tablet server hosting leader replica would be having a replicate op without
: // a corresponding COMMIT message
> Is there a way to ensure that's exactly the case? Maybe, use smallest poss
That should be the case, the reason being:
The first write arrives at the leader replica and immediately Raft replicated. Raft heartbeat interval is 500ms and we send a write request every 5 ms.
By the time the first few writes are sent to the followers and the leader replica is awaiting the ack, there would be a new write written to the WAL locally but not committed. By then we would shutdown the followers.
I did try with smaller sized (10 bytes) WALs and the number of the WALs was not consistently more in the leader replica although I could see 1 pending op after startup consistently.
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@400
PS6, Line 400: CHECK_OK
> Why not to use ASSERT_OK() here?
Done
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@405
PS6, Line 405: ASSERT_OK(InsertData(kNumRows, kNumRows * 2));
> Is there a way to check for
Incomplete comment?
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@410
PS6, Line 410: CHECK_OK
> Why not to use ASSERT_OK() here?
Done
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@411
PS6, Line 411: leader_ts_index + kNumTabletServers + 1
> Could this part be shorten to leader_ts_index + 1?
Yes, thanks for pointing that out.
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@419
PS6, Line 419: 3
> kNumTabletServers
Done
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@420
PS6, Line 420: 2
> kNumTabletServers - 1
Done
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/integration-tests/auto_incrementing-itest.cc@421
PS6, Line 421: ASSERT_EQ(results[i].size(), results[i+1].size());
: for (int k = 0; k < results[i].size(); k++) {
: ASSERT_EQ(results[i][k], results[i+1][k]);
: }
> Does it make sense to verify this across all the tablet replicas, not just
Yes, I changed the condition of the loop to be based on the number of tablet servers.
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/tablet/tablet.cc@437
PS6, Line 437: if (schema()->has_auto_incrementing()) {
> Consider moving this code out into a separate method.
Done
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/tablet/tablet.cc@442
PS6, Line 442: TODO(achennaka): Materialize only
> That's good point! Is it possible to do so in this patch or you are planni
It would be in a follow up patch. There doesn't seem to be too much of an overhead in this approach considering the time taken.
http://gerrit.cloudera.org:8080/#/c/19445/6/src/kudu/tablet/tablet.cc@466
PS6, Line 466: LOG_WITH_PREFIX(INFO) << "Fetching the auto incrementing counter took "
: << delta.ToMilliseconds() << "ms";
> It's better to use Stopwatch(Stopwatch::THIS_THREAD) for this purpose -- i
Done
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 6
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 29 Mar 2023 07:39:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 4:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/19445/5/src/kudu/integration-tests/auto_incrementing-itest.cc
File src/kudu/integration-tests/auto_incrementing-itest.cc:
http://gerrit.cloudera.org:8080/#/c/19445/5/src/kudu/integration-tests/auto_incrementing-itest.cc@415
PS5, Line 415: ERT_EQ(results[i].size(), resu
> nit: int i = 0; i < 3; i++
Done
http://gerrit.cloudera.org:8080/#/c/19445/5/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/19445/5/src/kudu/tablet/tablet.cc@423
PS5, Line 423: shared_ptr<DiskRowSet> rowset;
> warning: static member accessed through instance [readability-static-access
Done
http://gerrit.cloudera.org:8080/#/c/19445/5/src/kudu/tablet/tablet.cc@446
PS5, Line 446: {
> you remove the initial size 32K for mem
Right, but 32K is the default value
http://gerrit.cloudera.org:8080/#/c/19445/5/src/kudu/tablet/tablet.cc@466
PS5, Line 466:
> add a space before <<
Done
http://gerrit.cloudera.org:8080/#/c/19445/5/src/kudu/tablet/tablet.cc@466
PS5, Line 466: t_t
> AIC is not common abbreviation, not easy to understand.
Done
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 4
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Thu, 23 Mar 2023 18:27:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19445/6//COMMIT_MSG@38
PS6, Line 38: Columns - A Non Unique Primary Key column of type INT32
> If TS2 is started back as a follower replica, the in-memory auto incrementi
So, what if the first INSERTs are all went into one segment, and only the corresonding DELETEs are in the second, and the first WAL segment is GC-ed? Does anything changes in that case for TS2?
Thanks!
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 6
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 28 Mar 2023 05:38:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] [WIP]KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: [WIP]KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 3:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/19445/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19445/1//COMMIT_MSG@28
PS1, Line 28:
> It's nice to attach a tested metrics. Thanks
Yes, will add in the next patch.
http://gerrit.cloudera.org:8080/#/c/19445/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19445/2//COMMIT_MSG@14
PS2, Line 14: In this scenario the WAL segments are replayed and since each insert
: operation entry has auto incrementing counter which will be used for
: the insert operations present in that entry, as long as we have the
: latest insert operation entry in the WAL segments, the auto
: incrementing counter is populated correctly during bootstrap.
> Just wanted to make sure: is that working as expected when WAL contains non
Yes, it is but I will have a test case to cover this situation regardless.
http://gerrit.cloudera.org:8080/#/c/19445/2//COMMIT_MSG@20
PS2, Line 20: 2. WAL segments do not contain insert operations
: In the case, we need to fetch the highest auto incrementing counter
: value present in the data which is already flushed and update the
: in memory auto incrementing counter appropriately. This patch
: accomplishes this task.
> What if current snapshot of the data in the tablet doesn't have a single ro
Yes, in that case the counter starts from 1. I don't see any complications arising then.
http://gerrit.cloudera.org:8080/#/c/19445/2//COMMIT_MSG@25
PS2, Line 25: Change-Id: I61b305efd7d5a065a2
> It would be great to add test coverage to make sure this works as expected
Those test cases will be added. Just for this initial draft these were not included.
http://gerrit.cloudera.org:8080/#/c/19445/2/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/19445/2/src/kudu/tablet/tablet.cc@433
PS2, Line 433: pdate the auto incrementing count
> Would be great to add a comment why this is safe to look just at the curren
We would not allow for adding or dropping of the auto incrementing column. Hence this should be fine.
http://gerrit.cloudera.org:8080/#/c/19445/2/src/kudu/tablet/tablet.cc@440
PS2, Line 440: t->NewRow
> Could you add a comment to explain why 32K is used here for the row block m
Done
http://gerrit.cloudera.org:8080/#/c/19445/2/src/kudu/tablet/tablet.cc@449
PS2, Line 449:
> nit: drop the space ?
Done
http://gerrit.cloudera.org:8080/#/c/19445/2/src/kudu/tablet/tablet.cc@453
PS2, Line 453: if (counter > auto_incrementing_counter_) {
> I think this might look confusing. First, there might be be multiple diskr
This will be dropped. Was just set for my manual testing purposes.
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 3
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Mar 2023 07:52:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 9:
> Uploaded patch set 9.
It seems the new tests fails?
/home/jenkins-slave/workspace/kudu-master/0/src/kudu/integration-tests/auto_incrementing-itest.cc:310
Expected equality of these values:
Substitute("(int32 c0=$0, int64 $1=$2)", i + kNumRows, Schema::GetAutoIncrementingColumnName(), i + 1)
Which is: "(int32 c0=200, int64 auto_incrementing_id=1)"
results[i]
Which is: "(int32 c0=200, int64 auto_incrementing_id=201)"
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 9
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 31 Mar 2023 03:06:45 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Marton Greber, Tidy Bot, Alexey Serbin, Yingchun Lai, Kudu Jenkins, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19445
to look at the new patch set (#13).
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
KUDU-1945 Update auto incrementing counter during bootstrap
The auto incrementing counter would be reset to zero when the tablet
is being initialized. It is essential to have the correct value of
the counter. There are two cases:
1. WAL segments contain insert operations
In this scenario the WAL segments are replayed and since each insert
operation entry has auto incrementing counter which will be used for
the insert operations present in that entry, as long as we have the
latest insert operation entry in the WAL segments, the auto
incrementing counter is populated correctly during bootstrap.
2. WAL segments do not contain insert operations
In this case, we need to fetch the highest auto incrementing counter
value present in the data which is already flushed and update the
in-memory auto incrementing counter appropriately. This patch
accomplishes this task.
There are tests for the bootstrap scenarios where
1. We have no WAL segments with an INSERT OP containing the auto
incrementing column value. We rely on the auto incrementing counter
present in the data directories in this case.
2. We have no WAL segments with auto incrementing column value
and all the rows of the table are deleted. We reset the auto
incrementing counter in this case.
3. We have non committed replicate ops containing INSERT OPs with
the auto incrementing column values.
Manually tested the time taken to populate the auto incrementing
counter:
Columns - A Non Unique Primary Key column of type INT32
- 8 INT64 columns
- 5 STRING columns
Rows - 500k rows with data populated
Time taken in populating the counter during bootstrap:
Min - 235ms
Max - 466ms
Median - 312ms
The total time spent boostrapping the tablet was between 18-25
seconds.
Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
---
M src/kudu/integration-tests/auto_incrementing-itest.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tserver/tablet_server-test-base.h
4 files changed, 335 insertions(+), 42 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/19445/13
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 13
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Marton Greber, Tidy Bot, Alexey Serbin, Yingchun Lai, Kudu Jenkins, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19445
to look at the new patch set (#17).
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
KUDU-1945 Update auto incrementing counter during bootstrap
The auto incrementing counter would be reset to zero when the tablet
is being initialized. It is essential to have the correct value of
the counter. There are two cases:
1. WAL segments contain insert operations
In this scenario the WAL segments are replayed and since each insert
operation entry has auto incrementing counter which will be used for
the insert operations present in that entry, as long as we have the
latest insert operation entry in the WAL segments, the auto
incrementing counter is populated correctly during bootstrap.
2. WAL segments do not contain insert operations
In this case, we need to fetch the highest auto incrementing counter
value present in the data which is already flushed and update the
in-memory auto incrementing counter appropriately. This patch
accomplishes this task.
There are tests for the bootstrap scenarios where
1. We have no WAL segments with an INSERT OP containing the auto
incrementing column value. We rely on the auto incrementing counter
present in the data directories in this case.
2. We have no WAL segments with auto incrementing column value
and all the rows of the table are deleted. We reset the auto
incrementing counter in this case.
3. We have non committed replicate ops containing INSERT OPs with
the auto incrementing column values.
Manually tested the time taken to populate the auto incrementing
counter:
Columns - A Non Unique Primary Key column of type INT32
- 8 INT64 columns
- 5 STRING columns
Rows - 500k rows with data populated
Time taken in populating the counter during bootstrap:
Min - 235ms
Max - 466ms
Median - 312ms
The total time spent boostrapping the tablet was between 18-25
seconds.
Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
---
M src/kudu/integration-tests/auto_incrementing-itest.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tserver/tablet_server-test-base.h
4 files changed, 336 insertions(+), 42 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/19445/17
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 17
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 14:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19445/14/src/kudu/integration-tests/auto_incrementing-itest.cc
File src/kudu/integration-tests/auto_incrementing-itest.cc:
http://gerrit.cloudera.org:8080/#/c/19445/14/src/kudu/integration-tests/auto_incrementing-itest.cc@306
PS14, Line 306: // There could still be a WAL entry with a DELETE OP.
: // This might cause the auto incrementing counter to be not set to 0 unless
: // FlushDeltaMRS is called. We restart servers to force elections which in turn
: // writes more WAL entries which causes these flushes.
Wait, does it mean that in the wild we could end up in diverging counters for auto-incrementing column if a situation like in this scenario happens?
http://gerrit.cloudera.org:8080/#/c/19445/14/src/kudu/integration-tests/auto_incrementing-itest.cc@311
PS14, Line 311:
nit: remove an extra space
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 14
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Fri, 31 Mar 2023 19:55:35 +0000
Gerrit-HasComments: Yes
[kudu-CR] [WIP]KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: [WIP]KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19445/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19445/1//COMMIT_MSG@28
PS1, Line 28: 2. The patch itself
> This will slow down the bootstrap process due to additional processing. The
It's nice to attach a tested metrics. Thanks
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 2
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Sun, 29 Jan 2023 01:57:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Marton Greber, Tidy Bot, Alexey Serbin, Kudu Jenkins, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19445
to look at the new patch set (#10).
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
KUDU-1945 Update auto incrementing counter during bootstrap
The auto incrementing counter would be reset to zero when the tablet
is being initialized. It is essential to have the correct value of
the counter. There are two cases:
1. WAL segments contain insert operations
In this scenario the WAL segments are replayed and since each insert
operation entry has auto incrementing counter which will be used for
the insert operations present in that entry, as long as we have the
latest insert operation entry in the WAL segments, the auto
incrementing counter is populated correctly during bootstrap.
2. WAL segments do not contain insert operations
In this case, we need to fetch the highest auto incrementing counter
value present in the data which is already flushed and update the
in-memory auto incrementing counter appropriately. This patch
accomplishes this task.
There are tests for the bootstrap scenarios where
1. We have no WAL segments with an INSERT OP containing the auto
incrementing column value. We rely on the auto incrementing counter
present in the data directories in this case.
2. We have no WAL segments with auto incrementing column value
and all the rows of the table are deleted. We reset the auto
incrementing counter in this case.
3. We have non committed replicate ops containing INSERT OPs with
the auto incrementing column values.
Manually tested the time taken to populate the auto incrementing
counter:
Columns - A Non Unique Primary Key column of type INT32
- 8 INT64 columns
- 5 STRING columns
Rows - 500k rows with data populated
Time taken in populating the counter during bootstrap:
Min - 235ms
Max - 466ms
Median - 312ms
The total time spent boostrapping the tablet was between 18-25
seconds.
Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
---
M src/kudu/integration-tests/auto_incrementing-itest.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tserver/tablet_server-test-base.h
4 files changed, 328 insertions(+), 42 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/19445/10
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 10
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Marton Greber, Tidy Bot, Alexey Serbin, Kudu Jenkins, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19445
to look at the new patch set (#8).
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
KUDU-1945 Update auto incrementing counter during bootstrap
The auto incrementing counter would be reset to zero when the tablet
is being initialized. It is essential to have the correct value of
the counter. There are two cases:
1. WAL segments contain insert operations
In this scenario the WAL segments are replayed and since each insert
operation entry has auto incrementing counter which will be used for
the insert operations present in that entry, as long as we have the
latest insert operation entry in the WAL segments, the auto
incrementing counter is populated correctly during bootstrap.
2. WAL segments do not contain insert operations
In this case, we need to fetch the highest auto incrementing counter
value present in the data which is already flushed and update the
in-memory auto incrementing counter appropriately. This patch
accomplishes this task.
There are tests for the bootstrap scenarios where
1. We have no WAL segments with an INSERT OP containing the auto
incrementing column value. We rely on the auto incrementing counter
present in the data directories in this case.
2. We have no WAL segments with auto incrementing column value
and all the rows of the table are deleted. We reset the auto
incrementing counter in this case.
3. We have non committed replicate ops containing INSERT OPs with
the auto incrementing column values.
Manually tested the time taken to populate the auto incrementing
counter:
Columns - A Non Unique Primary Key column of type INT32
- 8 INT64 columns
- 5 STRING columns
Rows - 500k rows with data populated
Time taken in populating the counter during bootstrap:
Min - 235ms
Max - 466ms
Median - 312ms
The total time spent boostrapping the tablet was between 18-25
seconds.
Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
---
M src/kudu/integration-tests/auto_incrementing-itest.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tserver/tablet_server-test-base.h
4 files changed, 321 insertions(+), 42 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/19445/8
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 8
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Hello Marton Greber, Tidy Bot, Alexey Serbin, Kudu Jenkins, Wenzhe Zhou,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19445
to look at the new patch set (#12).
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
KUDU-1945 Update auto incrementing counter during bootstrap
The auto incrementing counter would be reset to zero when the tablet
is being initialized. It is essential to have the correct value of
the counter. There are two cases:
1. WAL segments contain insert operations
In this scenario the WAL segments are replayed and since each insert
operation entry has auto incrementing counter which will be used for
the insert operations present in that entry, as long as we have the
latest insert operation entry in the WAL segments, the auto
incrementing counter is populated correctly during bootstrap.
2. WAL segments do not contain insert operations
In this case, we need to fetch the highest auto incrementing counter
value present in the data which is already flushed and update the
in-memory auto incrementing counter appropriately. This patch
accomplishes this task.
There are tests for the bootstrap scenarios where
1. We have no WAL segments with an INSERT OP containing the auto
incrementing column value. We rely on the auto incrementing counter
present in the data directories in this case.
2. We have no WAL segments with auto incrementing column value
and all the rows of the table are deleted. We reset the auto
incrementing counter in this case.
3. We have non committed replicate ops containing INSERT OPs with
the auto incrementing column values.
Manually tested the time taken to populate the auto incrementing
counter:
Columns - A Non Unique Primary Key column of type INT32
- 8 INT64 columns
- 5 STRING columns
Rows - 500k rows with data populated
Time taken in populating the counter during bootstrap:
Min - 235ms
Max - 466ms
Median - 312ms
The total time spent boostrapping the tablet was between 18-25
seconds.
Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
---
M src/kudu/integration-tests/auto_incrementing-itest.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tserver/tablet_server-test-base.h
4 files changed, 337 insertions(+), 42 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/19445/12
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 12
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 12:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/19445/12/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/19445/12/src/kudu/tablet/tablet.cc@442
PS12, Line 442: "
> nit: How about output the failed reason, i.e. s.ToString().
Done
http://gerrit.cloudera.org:8080/#/c/19445/12/src/kudu/tablet/tablet.cc@496
PS12, Line 496: Stopwatch sw(Stopwatch::THIS_THREAD);
> How about using LOG_TIMING* macros for this to simplify code?
Done
http://gerrit.cloudera.org:8080/#/c/19445/12/src/kudu/tablet/tablet.cc@518
PS12, Line 518: if (counter > auto_incrementing_counter_) {
> Just curious, when counter <= auto_incrementing_counter_ ?
It doesn't get updated. We want to update the counter to the highest value seen.
counter <= auto_incrementing_counter_ might happen if the counter is already set to a high enough value from bootstrapping wal segments or if we have read a rowset with a higher value in previous iteration.
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 12
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Fri, 31 Mar 2023 17:08:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 15:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/19445/14/src/kudu/integration-tests/auto_incrementing-itest.cc
File src/kudu/integration-tests/auto_incrementing-itest.cc:
http://gerrit.cloudera.org:8080/#/c/19445/14/src/kudu/integration-tests/auto_incrementing-itest.cc@306
PS14, Line 306: // There could still be a WAL entry with a DELETE OP.
: // This might cause the auto incrementing counter to be not set to 0 unless
: // FlushDeltaMRS is called. We restart servers to force elections which in turn
: // writes more WAL entries which causes these flushes.
> Ah, OK -- for some reason I thought the check verification below was compar
The issue happens when the DELETE op is not persisted in the data directory. Having elections will ensure we have persisted all the DELETE ops in the data directory.
Updated the comment to be more specific.
http://gerrit.cloudera.org:8080/#/c/19445/16/src/kudu/integration-tests/auto_incrementing-itest.cc
File src/kudu/integration-tests/auto_incrementing-itest.cc:
http://gerrit.cloudera.org:8080/#/c/19445/16/src/kudu/integration-tests/auto_incrementing-itest.cc@294
PS16, Line 294: // We are still yet to GC the wal segments or flush the deletes,
: // check back the server after 50ms.
> nit: a misaligned comment
Done
http://gerrit.cloudera.org:8080/#/c/19445/16/src/kudu/integration-tests/auto_incrementing-itest.cc@310
PS16, Line 310: for (int i = 0; i < kNumTabletServers * 3; i++) {
> Instead, why not just shutdown all tablet servers, and then restart all of
Yes, that is the goal here.
http://gerrit.cloudera.org:8080/#/c/19445/16/src/kudu/integration-tests/auto_incrementing-itest.cc@311
PS16, Line 311: 3
> If doing this fancy stuff, this should be kNumTabletServers, not 3, right?
Done
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 15
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Sat, 01 Apr 2023 03:38:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 13:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19445/13/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/19445/13/src/kudu/tablet/tablet.cc@497
PS13, Line 497: <RowSet> &
nit: <RowSet>&
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 13
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Fri, 31 Mar 2023 17:13:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1945 Update auto incrementing counter during bootstrap
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/19445 )
Change subject: KUDU-1945 Update auto incrementing counter during bootstrap
......................................................................
Patch Set 14:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/19445/14/src/kudu/integration-tests/auto_incrementing-itest.cc
File src/kudu/integration-tests/auto_incrementing-itest.cc:
http://gerrit.cloudera.org:8080/#/c/19445/14/src/kudu/integration-tests/auto_incrementing-itest.cc@306
PS14, Line 306: // There could still be a WAL entry with a DELETE OP.
: // This might cause the auto incrementing counter to be not set to 0 unless
: // FlushDeltaMRS is called. We restart servers to force elections which in turn
: // writes more WAL entries which causes these flushes.
> Wait, does it mean that in the wild we could end up in diverging counters f
Yes, but since there would be only one leader at a given point of time, the next insert ops would receive either 0 or 200 (in this case) depending on which replica is the leader. There is not guarantee but there would be holes in the counter keyspace.
http://gerrit.cloudera.org:8080/#/c/19445/14/src/kudu/integration-tests/auto_incrementing-itest.cc@311
PS14, Line 311:
> nit: remove an extra space
Done
--
To view, visit http://gerrit.cloudera.org:8080/19445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I61b305efd7d5a065a2976327567163956c0c2184
Gerrit-Change-Number: 19445
Gerrit-PatchSet: 14
Gerrit-Owner: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Sat, 01 Apr 2023 00:04:18 +0000
Gerrit-HasComments: Yes