Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201610172105.u9HL5c0G032373@ip-10-146-233-104.ec2.internal>
Date: Mon, 17 Oct 2016 21:05:38 +0000
From: "Henry Robinson (Code Review)" <gerrit@cloudera.org>
To: Sailesh Mukil <sailesh@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Dan Hecht <dhecht@cloudera.com>, Matthew Jacobs <mj@cloudera.com>
Reply-To: henry@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-3823=3A_Add_timer_to_measure_Parquet_footer_reads=0A?=
In-Reply-To: <gerrit.1473563371000.Icf87bad90037dd0cea63b10c537382ec0f980cbf@gerrit.cloudera.org>
References: <gerrit.1473563371000.Icf87bad90037dd0cea63b10c537382ec0f980cbf@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Mon, 17 Oct 2016 21:05:43 -0000

Henry Robinson has posted comments on this change.

Change subject: IMPALA-3823: Add timer to measure Parquet footer reads
......................................................................


Patch Set 8: Code-Review+1

(11 comments)

http://gerrit.cloudera.org:8080/#/c/4371/8/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:

PS8, Line 204: Contrary to 
"Unlike"


PS8, Line 204: a 
remove


PS8, Line 206: The average is stored in the 'value_' member of the base class.
Referring to member variables in a class description is usually a sign the text is leaking implementation details. Do you mean to say that value() returns the average?


Line 209:   SummaryStatsCounter(TUnit::type unit, int32_t total_num_values,
Comment what this constructor is used for (is it when merging two SSCounters together?)


PS8, Line 216: total_num_values
can total_num_values be 0?


PS8, Line 247: This keeps track of t
Remove - just "The total..." is sufficient.


PS8, Line 255: SpinLock counter_lock_;
any reason not to call this lock_ ?


http://gerrit.cloudera.org:8080/#/c/4371/8/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:

PS8, Line 664: /* Print all SummaryStatsCounters as following:
             :     <Name>: (Avg: <value> ; Min: <min_value> ; Max: <max_value> ;
             :     Number of samples: <total>) */
Comment style?


PS8, Line 669: are
is


PS8, Line 1019: { min_ = new_value; }
remove parentheses for single-line if statements.


http://gerrit.cloudera.org:8080/#/c/4371/8/tests/query_test/test_scanners.py
File tests/query_test/test_scanners.py:

PS8, Line 317: \
remove


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Icf87bad90037dd0cea63b10c537382ec0f980cbf
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes