You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Anonymous Coward (Code Review)" <ge...@cloudera.org> on 2019/06/26 00:24:04 UTC

[kudu-CR] Modify table scan tool to collect and surface errors instead of crashing.

hannahvnguyen98@gmail.com has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13733


Change subject: Modify table scan tool to collect and surface errors instead of crashing.
......................................................................

Modify table scan tool to collect and surface errors instead of crashing.

Initialized a Status for each thread in StartWork(), passed as out-parameter to ScanTask() or CopyTask().
Changed signature of ScanData(): replaced CHECKs in ScanData() with return of bad Status.
ScanTask() or CopyTask() catch bad Statuses from ScanData() in out-parameter back to StartWork().
StartWork() logs and returns the first bad Status from the thread pool.

Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/table_scanner.cc
M src/kudu/tools/table_scanner.h
3 files changed, 61 insertions(+), 13 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
Gerrit-Change-Number: 13733
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <ha...@gmail.com>

[kudu-CR] KUDU-2851: modify table scan and copy tools to surface errors

Posted by "Hannah Nguyen (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, 

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

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

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

Change subject: KUDU-2851: modify table scan and copy tools to surface errors
......................................................................

KUDU-2851: modify table scan and copy tools to surface errors

- Initialized a Status for each thread.
- Replaced CHECKs in ScanData() with return of bad Status.
- StartWork() logs all bad Statuses, and returns the first bad Status
  from the thread pool.

Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/table_scanner.cc
M src/kudu/tools/table_scanner.h
3 files changed, 95 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/13733/6
-- 
To view, visit http://gerrit.cloudera.org:8080/13733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
Gerrit-Change-Number: 13733
Gerrit-PatchSet: 6
Gerrit-Owner: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] KUDU-2851: modify table scan and copy tools to surface errors

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

Change subject: KUDU-2851: modify table scan and copy tools to surface errors
......................................................................


Patch Set 8: -Code-Review

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13733/8/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/13733/8/src/kudu/tools/kudu-tool-test.cc@5037
PS8, Line 5037:   // Set flags so scans are rejected because they're not authorized.
              :   ASSERT_OK(cluster_->SetFlag(cluster_->tablet_server(0),
              :                               "tserver_enforce_access_control", "true"));
              :   ASSERT_OK(cluster_->SetFlag(cluster_->tablet_server(0),
              :                               "tserver_inject_invalid_authz_token_ratio", "1.0"));
Ah hrm, looks like the behavior of this isn't super reliable for failing the same way every time. Nor does it seem to make the test run any faster. Let's revert this to just shutting down the tablet server.

Same below.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
Gerrit-Change-Number: 13733
Gerrit-PatchSet: 8
Gerrit-Owner: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Thu, 27 Jun 2019 04:50:10 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2851: modify table scan and copy tools to surface errors

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

Change subject: KUDU-2851: modify table scan and copy tools to surface errors
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13733/7/src/kudu/tools/table_scanner.cc
File src/kudu/tools/table_scanner.cc:

http://gerrit.cloudera.org:8080/#/c/13733/7/src/kudu/tools/table_scanner.cc@468
PS7, Line 468: 
             :   Status scan_status = Status::OK();
             : 
             :   for (auto token : tokens) {
             :     Stopwatch sw(Stopwatch::THIS_THREAD);
             :     sw.start();
             : 
             :     KuduScanner* scanner_ptr;
             :     scan_status = token->IntoKuduScanner(&scanner_ptr);
             :     RETURN_NOT_OK(scan_status);
             : 
             :     unique_ptr<KuduScanner> scanner(scanner_ptr);
             :     scan_status = scanner->Open();
             :     RETURN_NOT_OK(scan_status);
             : 
             :     uint64_t count = 0;
             :     while (scanner->HasMoreRows()) {
             :       KuduScanBatch batch;
             :       scan_status = scanner->NextBatch(&batch);
             :       RETURN_NOT_OK(scan_status);
             :       count += batch.NumRows();
             :       total_count_.IncrementBy(batch.NumRows());
             :       cb(batch);
             :     }
             : 
             :     sw.stop();
             :     if (out_) {
             :       MutexLock l(output_lock_);
             :       *out_ << "T " << token->tablet().id() << " scanned count " << count
             :            << " cost " << sw.elapsed().wall_seconds() << " seconds" << endl;
             :     }
             :   }
             : 
             :   return scan_status;
A quick browse on this file, and I suggest to give up the local variable 'scan_status' since it's not necessary.^_^
For example:
-----------------------------------------
  for (auto token : tokens) {
    Stopwatch sw(Stopwatch::THIS_THREAD);
    sw.start();

    KuduScanner* scanner_ptr;
    RETURN_NOT_OK(token->IntoKuduScanner(&scanner_ptr));

    unique_ptr<KuduScanner> scanner(scanner_ptr);
    RETURN_NOT_OK(scanner->Open());

    uint64_t count = 0;
    while (scanner->HasMoreRows()) {
      KuduScanBatch batch;
      RETURN_NOT_OK(scanner->NextBatch(&batch));
      count += batch.NumRows();
      total_count_.IncrementBy(batch.NumRows());
      cb(batch);
    }

    sw.stop();
    if (out_) {
      MutexLock l(output_lock_);
      *out_ << "T " << token->tablet().id() << " scanned count " << count
           << " cost " << sw.elapsed().wall_seconds() << " seconds" << endl;
    }
  }

  return Status::OK();



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
Gerrit-Change-Number: 13733
Gerrit-PatchSet: 7
Gerrit-Owner: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Thu, 27 Jun 2019 01:31:09 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2851: modify table scan tool to surface errors instead of crashing

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

Change subject: KUDU-2851: modify table scan tool to surface errors instead of crashing
......................................................................


Patch Set 4:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/13733/3//COMMIT_MSG
Commit Message:

PS3: 
So it'll be clear when looking back through old commits, could you also mention the similar work you did for `table copy`?


http://gerrit.cloudera.org:8080/#/c/13733/3/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/13733/3/src/kudu/tools/kudu-tool-test.cc@5028
PS3, Line 5028: ser
nit: Can you remove the extra whitespace here? Same below, and above in patchset 4


http://gerrit.cloudera.org:8080/#/c/13733/3/src/kudu/tools/kudu-tool-test.cc@5035
PS3, Line 5035: enfo
nit: this is the copy tool :)


http://gerrit.cloudera.org:8080/#/c/13733/3/src/kudu/tools/table_scanner.cc
File src/kudu/tools/table_scanner.cc:

http://gerrit.cloudera.org:8080/#/c/13733/3/src/kudu/tools/table_scanner.cc@628
PS3, Line 628: 
             :   for (i = 0; i < FLAGS_num_threads; ++i) {
             :     if (!thread_statuses[i].ok() && out_) {
             :       *out_ << "Scanning failed " << thread_statuses[i].ToString() << endl;
             :       if (end_status.ok()) end_status = thread_statuses[i];
             :     }
             :   }
Looking at TableScanner's out_ a bit, it's possible that `out_` never gets set via SetOutput(). In such cases, `out_` will be nullptr, and this will be skipped. Could you tweak this a bit so `end_status` gets set correctly, regardless of whether `out_` is set or not?

Also, minor nit: could you move this Status-parsing and printing so it comes after `sw.stop()`? While generally pretty negligible, printing and branches do take time, and it'd be nice to entirely separate this from the scan being timed.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
Gerrit-Change-Number: 13733
Gerrit-PatchSet: 4
Gerrit-Owner: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 26 Jun 2019 23:31:34 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2851: modify table scan and copy tools to surface errors

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

Change subject: KUDU-2851: modify table scan and copy tools to surface errors
......................................................................


Patch Set 9: Code-Review+2

Thanks for helping review, Lifu!


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
Gerrit-Change-Number: 13733
Gerrit-PatchSet: 9
Gerrit-Owner: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Thu, 27 Jun 2019 17:59:02 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2851: modify table scan and copy tools to surface errors

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

Change subject: KUDU-2851: modify table scan and copy tools to surface errors
......................................................................


Patch Set 5: Code-Review+2

(2 comments)

http://gerrit.cloudera.org:8080/#/c/13733/3//COMMIT_MSG
Commit Message:

PS3: 
> I thought I changed this in PS5?
Ah you're right. My bad!


http://gerrit.cloudera.org:8080/#/c/13733/3/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/13733/3/src/kudu/tools/kudu-tool-test.cc@5035
PS3, Line 5035: enfo
> Also thought this was changed in PS5?
Ah you're right. My bad!



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
Gerrit-Change-Number: 13733
Gerrit-PatchSet: 5
Gerrit-Owner: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 27 Jun 2019 00:30:28 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2851: modify table scan and copy tools to surface errors

Posted by "Hannah Nguyen (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, 

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

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

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

Change subject: KUDU-2851: modify table scan and copy tools to surface errors
......................................................................

KUDU-2851: modify table scan and copy tools to surface errors

- Initialized a Status for each thread.
- Replaced CHECKs in ScanData() with return of bad Status.
- StartWork() logs all bad Statuses, and returns the first bad Status
  from the thread pool.

Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/table_scanner.cc
M src/kudu/tools/table_scanner.h
3 files changed, 95 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/13733/5
-- 
To view, visit http://gerrit.cloudera.org:8080/13733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
Gerrit-Change-Number: 13733
Gerrit-PatchSet: 5
Gerrit-Owner: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] KUDU-2851: modify table scan and copy tools to surface errors

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

Change subject: KUDU-2851: modify table scan and copy tools to surface errors
......................................................................


Patch Set 8: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
Gerrit-Change-Number: 13733
Gerrit-PatchSet: 8
Gerrit-Owner: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Thu, 27 Jun 2019 02:57:09 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2851: modify table scan and copy tools to surface errors

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

Change subject: KUDU-2851: modify table scan and copy tools to surface errors
......................................................................


Patch Set 7: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
Gerrit-Change-Number: 13733
Gerrit-PatchSet: 7
Gerrit-Owner: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 27 Jun 2019 00:58:37 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2851: modify table scan and copy tools to surface errors

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

Change subject: KUDU-2851: modify table scan and copy tools to surface errors
......................................................................

KUDU-2851: modify table scan and copy tools to surface errors

- Initialized a Status for each thread.
- Replaced CHECKs in ScanData() with return of bad Status.
- StartWork() logs all bad Statuses, and returns the first bad Status
  from the thread pool.

Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
Reviewed-on: http://gerrit.cloudera.org:8080/13733
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/table_scanner.cc
M src/kudu/tools/table_scanner.h
3 files changed, 87 insertions(+), 16 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Andrew Wong: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
Gerrit-Change-Number: 13733
Gerrit-PatchSet: 10
Gerrit-Owner: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>

[kudu-CR] KUDU-2851: modify table scan and copy tools to surface errors

Posted by "Hannah Nguyen (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, 

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

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

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

Change subject: KUDU-2851: modify table scan and copy tools to surface errors
......................................................................

KUDU-2851: modify table scan and copy tools to surface errors

- Initialized a Status for each thread.
- Replaced CHECKs in ScanData() with return of bad Status.
- StartWork() logs all bad Statuses, and returns the first bad Status
  from the thread pool.

Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/table_scanner.cc
M src/kudu/tools/table_scanner.h
3 files changed, 94 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/13733/7
-- 
To view, visit http://gerrit.cloudera.org:8080/13733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
Gerrit-Change-Number: 13733
Gerrit-PatchSet: 7
Gerrit-Owner: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] KUDU-2851: modify table scan and copy tools to surface errors

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

Change subject: KUDU-2851: modify table scan and copy tools to surface errors
......................................................................


Patch Set 7:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/13733/7/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/13733/7/src/kudu/tools/kudu-tool-test.cc@5030
PS7, Line 5030: TEST_F(ToolTest, TestFailedTableScan) {
> error: static data member 'test_info_' not allowed in local class 'ToolTest
Done


http://gerrit.cloudera.org:8080/#/c/13733/7/src/kudu/tools/kudu-tool-test.cc@5056
PS7, Line 5056: TEST_F(ToolTest, TestFailedTableCopy) {
> error: function definition is not allowed here [clang-diagnostic-error]
Done


http://gerrit.cloudera.org:8080/#/c/13733/7/src/kudu/tools/kudu-tool-test.cc@5088
PS7, Line 5088: } // namespace kudu
> error: expected '}' [clang-diagnostic-error]
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
Gerrit-Change-Number: 13733
Gerrit-PatchSet: 7
Gerrit-Owner: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Thu, 27 Jun 2019 02:30:02 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2851: modify table scan and copy tools to surface errors

Posted by "Hannah Nguyen (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, helifu, 

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

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

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

Change subject: KUDU-2851: modify table scan and copy tools to surface errors
......................................................................

KUDU-2851: modify table scan and copy tools to surface errors

- Initialized a Status for each thread.
- Replaced CHECKs in ScanData() with return of bad Status.
- StartWork() logs all bad Statuses, and returns the first bad Status
  from the thread pool.

Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/table_scanner.cc
M src/kudu/tools/table_scanner.h
3 files changed, 91 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/13733/8
-- 
To view, visit http://gerrit.cloudera.org:8080/13733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
Gerrit-Change-Number: 13733
Gerrit-PatchSet: 8
Gerrit-Owner: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>

[kudu-CR] KUDU-2851: modify table scan and copy tools to surface errors

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

Change subject: KUDU-2851: modify table scan and copy tools to surface errors
......................................................................


Patch Set 5:

Seems like there's a merge conflict with some code that landed recently. Could you rebase this change on the tip of master?


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
Gerrit-Change-Number: 13733
Gerrit-PatchSet: 5
Gerrit-Owner: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 27 Jun 2019 00:33:59 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2851: modify table scan and copy tools to surface errors

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

Change subject: KUDU-2851: modify table scan and copy tools to surface errors
......................................................................


Patch Set 5: Code-Review+1

(2 comments)

http://gerrit.cloudera.org:8080/#/c/13733/3//COMMIT_MSG
Commit Message:

PS3: 
> Done
Missed this?


http://gerrit.cloudera.org:8080/#/c/13733/3/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/13733/3/src/kudu/tools/kudu-tool-test.cc@5035
PS3, Line 5035: enfo
> Done
Missed this?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
Gerrit-Change-Number: 13733
Gerrit-PatchSet: 5
Gerrit-Owner: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 27 Jun 2019 00:21:36 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2851: modify table scan and copy tools to surface errors

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

Change subject: KUDU-2851: modify table scan and copy tools to surface errors
......................................................................


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/13733/3//COMMIT_MSG
Commit Message:

PS3: 
> Done
I thought I changed this in PS5?


http://gerrit.cloudera.org:8080/#/c/13733/3/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/13733/3/src/kudu/tools/kudu-tool-test.cc@5035
PS3, Line 5035: enfo
> Done
Also thought this was changed in PS5?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
Gerrit-Change-Number: 13733
Gerrit-PatchSet: 5
Gerrit-Owner: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 27 Jun 2019 00:26:28 +0000
Gerrit-HasComments: Yes

[kudu-CR] Modify table scan tool to collect and surface errors instead of crashing

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

Change subject: Modify table scan tool to collect and surface errors instead of crashing
......................................................................


Patch Set 2:

(11 comments)

Looks pretty good!

http://gerrit.cloudera.org:8080/#/c/13733/1//COMMIT_MSG
Commit Message:

PS1: 
nit: For commit messages, we try to follow the guidance outlined here:

https://chris.beams.io/posts/git-commit/


http://gerrit.cloudera.org:8080/#/c/13733/1//COMMIT_MSG@7
PS1, Line 7: Modify table scan tool to collect and surface errors instead of crashing
nit: Could you prefix this with KUDU-2851 so it's easy to associate this patch with the ticket? e.g.

KUDU-2851: modify table scan tool to collect and surface errors instead of crashing


http://gerrit.cloudera.org:8080/#/c/13733/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/13733/2//COMMIT_MSG@9
PS2, Line 9: Initialized a Status for each thread in StartWork().
           : Replaced CHECKs in ScanData() with return of bad Status.
           : ScanTask() or CopyTask() catch bad Statuses from ScanData() in 
           : out-parameter back to StartWork().
           : StartWork() logs and returns the first bad Status from the thread pool.
nit: Since this is a list of stuff, could you maybe add bullets (e.g. *s or -s) before each item, and adjust the spacing so it's a little easier to read? E.g.

...
- ScanTask() or Copy...
  out-parameter back...
- StartWork() logs a...

Also, these are good details, but you probably don't _need_ to go _that_ in-depth to the point where you're calling out specific functions. A high-level description of the approach should suffice.


http://gerrit.cloudera.org:8080/#/c/13733/1/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/13733/1/src/kudu/tools/kudu-tool-test.cc@5004
PS1, Line 5004:   // Now shut down the tablet servers so the scans cannot proceed.
              :   // Upon running the scan tool, we should get a TimedOut status.
              :   NO_FATALS(cluster_->ShutdownNodes(cluster::ClusterNodes::TS_ONLY));
The goal here was to make sure that our scan hit an error, and shutting down the Tablet Servers was a means to that end. An side effect of that decision, as you saw, is that we have to wait for our scan to time out, which can take a while. Instead, how about we use a different way to inject errors to the scan?

Let's instead set the following flags on the Tablet Servers to inject errors:
- tserver_enforce_access_control=true
- tserver_inject_invalid_authz_token_ratio=1.0

Rather than having to wait for a timeout, this will have the scans make their ways to the Tablet Servers, and immediately be rejected because they're not authorized.

An example of setting flags on a cluster can be found in integration-tests/disk_failure-itest.cc at L203.


http://gerrit.cloudera.org:8080/#/c/13733/1/src/kudu/tools/table_scanner.h
File src/kudu/tools/table_scanner.h:

http://gerrit.cloudera.org:8080/#/c/13733/1/src/kudu/tools/table_scanner.h@84
PS1, Line 84:                 const std::function<void(const kudu::client::KuduScanBatch& batch)>& cb);
nit: Could you fix the spacing alignment here?


http://gerrit.cloudera.org:8080/#/c/13733/1/src/kudu/tools/table_scanner.cc
File src/kudu/tools/table_scanner.cc:

http://gerrit.cloudera.org:8080/#/c/13733/1/src/kudu/tools/table_scanner.cc@467
PS1, Line 467:                             const std::function<void(const KuduScanBatch& batch)>& cb) {
nit: could you fix the spacing alignment here?


http://gerrit.cloudera.org:8080/#/c/13733/1/src/kudu/tools/table_scanner.cc@476
PS1, Line 476:     scan_status = token->IntoKuduScanner(&scanner_ptr);
             :     if (!scan_status.ok()) return scan_status;
nit: You can use the RETURN_NOT_OK macro and it does this for you.


http://gerrit.cloudera.org:8080/#/c/13733/1/src/kudu/tools/table_scanner.cc@605
PS1, Line 605:   vector<Status> thread_statuses;
             :   for (i = 0; i < FLAGS_num_threads; ++i) {
             :     thread_statuses.emplace_back(Status::OK());
             :   }
nit: The default constructor for a Status object in Kudu is Status::OK(). If you supply an int to a vector's constructor, it'll initialize the members with the default constructor. E.g.

  vector<Status> thread_statuses(FLAGS_num_threads);


http://gerrit.cloudera.org:8080/#/c/13733/1/src/kudu/tools/table_scanner.cc@624
PS1, Line 624:         boost::bind(&TableScanner::CopyTask, this, thread_tokens[i], &thread_statuses[i])));
Since you're fixing this too, could you add a similar test for copying? Alternatively, don't do this here and you could perhaps address this in a second patch.


http://gerrit.cloudera.org:8080/#/c/13733/1/src/kudu/tools/table_scanner.cc@631
PS1, Line 631:   for (i = 0; i < FLAGS_num_threads; ++i) {
There's a decent amount of stuff happening in this function. One thing of note is that we're timing how long it takes to scan. The end of this timing period is specified by sw.stop(), and then a timing message based on the scan is output below.

Rather than exiting early, let's still print out the scanned rows and the timing info, but _additionally_ print out any errors and returning the first bad one.


http://gerrit.cloudera.org:8080/#/c/13733/1/src/kudu/tools/table_scanner.cc@633
PS1, Line 633:       LOG(INFO) << "Scanning failed " << thread_statuses[i].ToString() << endl;
In tooling, we prefer to log to stdout. Here, that would be *out_.

Also, instead of printing the first error, I think it'd be more usable to print out all of the errors, e.g. one on each line. WDYT?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
Gerrit-Change-Number: 13733
Gerrit-PatchSet: 2
Gerrit-Owner: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 26 Jun 2019 00:59:00 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2851: modify table scan and copy tools to surface errors

Posted by "Hannah Nguyen (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, helifu, 

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

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

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

Change subject: KUDU-2851: modify table scan and copy tools to surface errors
......................................................................

KUDU-2851: modify table scan and copy tools to surface errors

- Initialized a Status for each thread.
- Replaced CHECKs in ScanData() with return of bad Status.
- StartWork() logs all bad Statuses, and returns the first bad Status
  from the thread pool.

Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/table_scanner.cc
M src/kudu/tools/table_scanner.h
3 files changed, 87 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/13733/9
-- 
To view, visit http://gerrit.cloudera.org:8080/13733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
Gerrit-Change-Number: 13733
Gerrit-PatchSet: 9
Gerrit-Owner: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>

[kudu-CR] KUDU-2851: modify table scan tool to surface errors instead of crashing

Posted by "Hannah Nguyen (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, 

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

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

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

Change subject: KUDU-2851: modify table scan tool to surface errors instead of crashing
......................................................................

KUDU-2851: modify table scan tool to surface errors instead of crashing

- Initialized a Status for each thread.
- Replaced CHECKs in ScanData() with return of bad Status.
- StartWork() logs all bad Statuses, and returns the first bad Status
  from the thread pool.

Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/table_scanner.cc
M src/kudu/tools/table_scanner.h
3 files changed, 91 insertions(+), 16 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
Gerrit-Change-Number: 13733
Gerrit-PatchSet: 3
Gerrit-Owner: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] Modify table scan tool to collect and surface errors instead of crashing

Posted by "Hannah Nguyen (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, 

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

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

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

Change subject: Modify table scan tool to collect and surface errors instead of crashing
......................................................................

Modify table scan tool to collect and surface errors instead of crashing

Initialized a Status for each thread in StartWork().
Replaced CHECKs in ScanData() with return of bad Status.
ScanTask() or CopyTask() catch bad Statuses from ScanData() in 
out-parameter back to StartWork().
StartWork() logs and returns the first bad Status from the thread pool.

Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/table_scanner.cc
M src/kudu/tools/table_scanner.h
3 files changed, 61 insertions(+), 13 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
Gerrit-Change-Number: 13733
Gerrit-PatchSet: 2
Gerrit-Owner: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] KUDU-2851: modify table scan tool to surface errors instead of crashing

Posted by "Hannah Nguyen (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, 

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

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

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

Change subject: KUDU-2851: modify table scan tool to surface errors instead of crashing
......................................................................

KUDU-2851: modify table scan tool to surface errors instead of crashing

- Initialized a Status for each thread.
- Replaced CHECKs in ScanData() with return of bad Status.
- StartWork() logs all bad Statuses, and returns the first bad Status
  from the thread pool.

Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/table_scanner.cc
M src/kudu/tools/table_scanner.h
3 files changed, 95 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/13733/4
-- 
To view, visit http://gerrit.cloudera.org:8080/13733
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
Gerrit-Change-Number: 13733
Gerrit-PatchSet: 4
Gerrit-Owner: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] KUDU-2851: modify table scan and copy tools to surface errors

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

Change subject: KUDU-2851: modify table scan and copy tools to surface errors
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13733/7/src/kudu/tools/table_scanner.cc
File src/kudu/tools/table_scanner.cc:

http://gerrit.cloudera.org:8080/#/c/13733/7/src/kudu/tools/table_scanner.cc@468
PS7, Line 468: 
             :   Status scan_status = Status::OK();
             : 
             :   for (auto token : tokens) {
             :     Stopwatch sw(Stopwatch::THIS_THREAD);
             :     sw.start();
             : 
             :     KuduScanner* scanner_ptr;
             :     scan_status = token->IntoKuduScanner(&scanner_ptr);
             :     RETURN_NOT_OK(scan_status);
             : 
             :     unique_ptr<KuduScanner> scanner(scanner_ptr);
             :     scan_status = scanner->Open();
             :     RETURN_NOT_OK(scan_status);
             : 
             :     uint64_t count = 0;
             :     while (scanner->HasMoreRows()) {
             :       KuduScanBatch batch;
             :       scan_status = scanner->NextBatch(&batch);
             :       RETURN_NOT_OK(scan_status);
             :       count += batch.NumRows();
             :       total_count_.IncrementBy(batch.NumRows());
             :       cb(batch);
             :     }
             : 
             :     sw.stop();
             :     if (out_) {
             :       MutexLock l(output_lock_);
             :       *out_ << "T " << token->tablet().id() << " scanned count " << count
             :            << " cost " << sw.elapsed().wall_seconds() << " seconds" << endl;
             :     }
             :   }
             : 
             :   return scan_status;
> A quick browse on this file, and I suggest to give up the local variable 's
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
Gerrit-Change-Number: 13733
Gerrit-PatchSet: 7
Gerrit-Owner: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Thu, 27 Jun 2019 02:29:28 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2851: modify table scan and copy tools to surface errors

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

Change subject: KUDU-2851: modify table scan and copy tools to surface errors
......................................................................


Patch Set 5:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/13733/3//COMMIT_MSG
Commit Message:

PS3: 
> So it'll be clear when looking back through old commits, could you also men
Done


http://gerrit.cloudera.org:8080/#/c/13733/3/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/13733/3/src/kudu/tools/kudu-tool-test.cc@5028
PS3, Line 5028: ser
> nit: Can you remove the extra whitespace here? Same below, and above in pat
Done


http://gerrit.cloudera.org:8080/#/c/13733/3/src/kudu/tools/kudu-tool-test.cc@5035
PS3, Line 5035: enfo
> nit: this is the copy tool :)
Done


http://gerrit.cloudera.org:8080/#/c/13733/3/src/kudu/tools/table_scanner.cc
File src/kudu/tools/table_scanner.cc:

http://gerrit.cloudera.org:8080/#/c/13733/3/src/kudu/tools/table_scanner.cc@628
PS3, Line 628: 
             :   sw.stop();
             :   if (out_) {
             :     *out_ << "Total count " << total_count_.Load()
             :         << " cost " << sw.elapsed().wall_seconds() << " seconds" << endl;
             :   }
             : 
> Looking at TableScanner's out_ a bit, it's possible that `out_` never gets 
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic45da537b8bacfa9625010536ea82da9a6e76100
Gerrit-Change-Number: 13733
Gerrit-PatchSet: 5
Gerrit-Owner: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hannah Nguyen <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 27 Jun 2019 00:18:32 +0000
Gerrit-HasComments: Yes