Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201710050027.v950Rdm6011184@ip-10-146-233-104.ec2.internal>
Date: Thu, 5 Oct 2017 00:27:39 +0000
From: "John Russell (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Tim Armstrong <tarmstrong@cloudera.com>,
        Mostafa Mokhtar <mmokhtar@cloudera.com>
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-3200=3A_=5BDOCS=5D_Document_user-facing_aspects_of_new_buffer_pool=0A?=
In-Reply-To: <gerrit.1504818934000.I49323f8ffbff3e195058e88762eedbb1fcb1bc0e@gerrit.cloudera.org>
References: <gerrit.1504818934000.I49323f8ffbff3e195058e88762eedbb1fcb1bc0e@gerrit.cloudera.org>
Reply-To: jrussell@cloudera.com, impala-cr@cloudera.com, marcelk@gmail.com,
        tarmstrong@cloudera.com, mmokhtar@cloudera.com,
        reviews@impala.incubator.apache.org, dhecht@cloudera.com
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.14.2
Content-Type: multipart/alternative; boundary="qW51RZVsd2M="; charset=UTF-8
archived-at: Thu, 05 Oct 2017 00:27:47 -0000

--qW51RZVsd2M=
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

John Russell has posted comments on this change=2E ( http://gerrit=2Ecloude=
ra=2Eorg:8080/8003 )

Change subject: IMPALA-3200: [DOCS] Document user-fac=
ing aspects of new buffer pool
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=
=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=2E=
=2E=2E=2E=2E=2E=2E


Patch Set 4:

(21 comments)

http://gerrit=2Ecloudera=
=2Eorg:8080/#/c/8003/4/docs/topics/impala_buffer_pool_limit=2Exml
File docs=
/topics/impala_buffer_pool_limit=2Exml:

http://gerrit=2Ecloudera=2Eorg:808=
0/#/c/8003/4/docs/topics/impala_buffer_pool_limit=2Exml@39
PS4, Line 39:   =
    Defines the size in bytes of an internal buffer for allocating memory d=
uring queries=2E
> First sentence isn't accurate=2E Something like this mig=
ht be better: "Define
Done


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8003/4=
/docs/topics/impala_buffer_pool_limit=2Exml@53
PS4, Line 53: MiB
MB=2E I ma=
y have mixed up MB / KB and MiB / KiB in a few places throughout=2E MB / KB=
 =3D powers of 2, MiB / KiB =3D powers of 10=2E


http://gerrit=2Ecloudera=
=2Eorg:8080/#/c/8003/4/docs/topics/impala_buffer_pool_limit=2Exml@53
PS4, L=
ine 53: his default is represented by a value
            :       of 0=2E
>=
 The default is actually that the query option is unset=2E 0 means a limit =
of 
Done


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8003/4/docs/topics/impal=
a_buffer_pool_limit=2Exml@69
PS4, Line 69: set buffer_pool_limit=3D8GB;
> I=
t can also be expressed as a percentage of mem_limit (e=2Eg=2E 80%), which =
may
Done


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8003/4/docs/topics/impal=
a_default_spillable_buffer_size=2Exml
File docs/topics/impala_default_spill=
able_buffer_size=2Exml:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8003/4/doc=
s/topics/impala_default_spillable_buffer_size=2Exml@44
PS4, Line 44:     <p=
 conref=3D"=2E=2E/shared/impala_common=2Exml#common/type_integer"/>
> Do we=
 document somewhere the supported byte suffixes (e=2Eg=2E B, KB, MB, GB pl
=
Done=2E I'll reuse wording to that effect from the MEM_LIMIT page=2E I trus=
t that of these new query options, only BUFFER_LIMIT accepts a percentage, =
the others are purely sizes in bytes, correct?


http://gerrit=2Ecloudera=
=2Eorg:8080/#/c/8003/4/docs/topics/impala_default_spillable_buffer_size=2Ex=
ml@49
PS4, Line 49:     <p>
> I think it's confusing to say that the defaul=
t varies, since the query opti
Done


http://gerrit=2Ecloudera=2Eorg:8080/#=
/c/8003/4/docs/topics/impala_default_spillable_buffer_size=2Exml@50
PS4, Li=
ne 50:       <codeph>65536</codeph> to <codeph>2097152</codeph>, depending =
on the
> Feel free to ignore, but might be easier to read if they were huma=
n-readabl
Done


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8003/4/docs/topics=
/impala_default_spillable_buffer_size=2Exml@59
PS4, Line 59:       consider=
 increasing the <codeph>DEFAULT_SPILLABLE_BUFFER_SIZE</codeph> setting=2E
>=
 Would it be helpful to elaborate here? E=2Eg=2E "Larger buffer sizes will =
resul
Done


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8003/4/docs/topics/imp=
ala_default_spillable_buffer_size=2Exml@61
PS4, Line 61:     </p>
> It's im=
plied but would it make sense to state the inverse=2E I=2Ee=2E decreasing 
=
Done


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8003/4/docs/topics/impala_ma=
x_row_size=2Exml
File docs/topics/impala_max_row_size=2Exml:

http://gerrit=
=2Ecloudera=2Eorg:8080/#/c/8003/4/docs/topics/impala_max_row_size=2Exml@47
=
PS4, Line 47: 524288
> 512K to be human-readable?
Done


http://gerrit=2Ecl=
oudera=2Eorg:8080/#/c/8003/4/docs/topics/impala_max_row_size=2Exml@56
PS4, =
Line 56: accomodate
> sp: accommodate
That and 'separate' vs=2E 'seperate' =
are my kryptonite=2E


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8003/4/docs/=
topics/impala_max_row_size=2Exml@57
PS4, Line 57:       the total bytes sto=
red in the largest row=2E
> It's left ambiguous whether queries processing =
rows larger than MAX_ROW_SIZ
Good point=2E In my testing I tried to come cl=
ose to the boundary with a 530 K row size and was surprised that the query =
succeeded=2E

Oops, I see this ties in to the initial description of the op=
tion purpose, which I left blank by mistake=2E I'll fill that in above=2E

=

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8003/4/docs/topics/impala_max_row_=
size=2Exml@82
PS4, Line 82: 500 KB string and a 30 KB
These are actually Ki=
B=2E I didn't mean to actually cut it so close (530,000 vs=2E 524,288 limit=
)=2E I couldn't make this slightly-too-large row cause a failure=2E


http:=
//gerrit=2Ecloudera=2Eorg:8080/#/c/8003/4/docs/topics/impala_max_row_size=
=2Exml@95
PS4, Line 95: KiB
Agh, KiB again where it should be KB=2E


http:=
//gerrit=2Ecloudera=2Eorg:8080/#/c/8003/4/docs/topics/impala_max_row_size=
=2Exml@109
PS4, Line 109: Increase the max_row_size query option (currently=
 512=2E00 KB) to process larger rows=2E
Wrap these very long error message =
lines=2E


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8003/4/docs/topics/impal=
a_min_spillable_buffer_size=2Exml
File docs/topics/impala_min_spillable_buf=
fer_size=2Exml:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8003/4/docs/topics=
/impala_min_spillable_buffer_size=2Exml@38
PS4, Line 38:       <indexterm a=
udience=3D"hidden">MIN_SPILLABLE_BUFFER_SIZE query option</indexterm>
> I t=
hink most of the comments I made on DEFAULT_SPILLABLE_BUFFER_SIZE have a
Do=
ne=2E Please doublecheck whether the wording I lifted from the DEFAULT_ pag=
e also makes sense here=2E E=2Eg=2E is it even possible to lower the MIN_ s=
etting to less than 64 KB, or could you only ever increase it?


http://ger=
rit=2Ecloudera=2Eorg:8080/#/c/8003/4/docs/topics/impala_scalability=2Exml
F=
ile docs/topics/impala_scalability=2Exml:

http://gerrit=2Ecloudera=2Eorg:8=
080/#/c/8003/4/docs/topics/impala_scalability=2Exml@425
PS4, Line 425: allo=
cated
> it's reserved at the beginning of the query, not allocated=2E The d=
istinction
Done


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8003/4/docs/topic=
s/impala_scalability=2Exml@560
PS4, Line 560:         The infrastructure of=
 the spilling feature affects the way the affected SQL operators, such as
>=
 A lot of the discussion in this paragraph and the one below is describing =
t
OK, I will summarize the new situation and then comment out the old discu=
ssion (in case we need to rescue any details from it later=2E)


http://ger=
rit=2Ecloudera=2Eorg:8080/#/c/8003/4/docs/topics/impala_scalability=2Exml@6=
15
PS4, Line 615: >BlockMgr=2EBytesWritten
> This still needs updating - IM=
PALA-5656 - should I keep an eye out for a di
Nah, there's only a single re=
ference to this name so I'll fix IMPALA-5656 as part of this gerrit=2E


ht=
tp://gerrit=2Ecloudera=2Eorg:8080/#/c/8003/4/docs/topics/impala_scalability=
=2Exml@701
PS4, Line 701:         <b>Testing performance implications of sp=
illing to disk:</b>
> This section is stale too=2E The scenario still seems=
 valid but the profile o
OK, I'll skip over the output piece and see if use=
rs ask for more details to be added back=2E


http://gerrit=2Ecloudera=2Eor=
g:8080/#/c/8003/4/docs/topics/impala_scalability=2Exml@745
PS4, Line 745: D=
o not specify a memory limit lower than about 300 MB, because with such a
 =
            :         low limit, queries could fail to start for other reas=
ons=2E Now try the memory-intensive query again=2E
> This is one of the thi=
ngs that was fixed :)
Done


-- 
To view, visit http://gerrit=2Ecloudera=
=2Eorg:8080/8003
To unsubscribe, visit http://gerrit=2Ecloudera=2Eorg:8080/=
settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageTy=
pe: comment
Gerrit-Change-Id: I49323f8ffbff3e195058e88762eedbb1fcb1bc0e
Ger=
rit-Change-Number: 8003
Gerrit-PatchSet: 4
Gerrit-Owner: John Russell <jrus=
sell@cloudera=2Ecom>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera=2Ecom>
Ger=
rit-Reviewer: John Russell <jrussell@cloudera=2Ecom>
Gerrit-Reviewer: Mosta=
fa Mokhtar <mmokhtar@cloudera=2Ecom>
Gerrit-Reviewer: Tim Armstrong <tarmst=
rong@cloudera=2Ecom>
Gerrit-Comment-Date: Thu, 05 Oct 2017 00:27:39 +0000
G=
errit-HasComments: Yes

--qW51RZVsd2M=--