date:20161129

[Impala-ASF-CR] IMPALA-4543: Properly escape ignored tests subdirectories.

2016-11-29 Thread Michael Ho (Code Review)

Michael Ho has posted comments on this change.

Change subject: IMPALA-4543: Properly escape ignored tests subdirectories.
..


Patch Set 1:

(2 comments)

Just wondering why we started hitting that ? Why isn't it showing up in other 
Jenkins jobs ?

http://gerrit.cloudera.org:8080/#/c/5242/1/tests/run-tests.py
File tests/run-tests.py:

PS1, Line 68: print >> sys.stderr, "run_tests arguments:", args
sys.stderr.write('Unexpected exception with pytest %s' % args)


PS1, Line 144: '\''
Is this supposed to be \' instead ?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I006eb559ec5f5b5b0379997fab945116dfc7e8d7
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading

2016-11-29 Thread Bharath Vissapragada (Code Review)

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
..


Patch Set 13: Code-Review+1

Thanks for the review Alex. Carrying +1.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading

2016-11-29 Thread Bharath Vissapragada (Code Review)

Hello Alex Behm,

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

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

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

Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
..

IMPALA-4172/IMPALA-3653: Improvements to block metadata loading

This patch improves the block metadata loading (locations and disk
storage IDs) for partitioned and un-partitioned tables in the Catalog
server.

Without this patch:
--
We loop through each and every file in the table/partition directories
and call getFileBlockLocations() on it to obtain the block metadata.
This results in large number of RPC calls to the Namenode, especially
for tables with large no. of files/partitions.

With this patch:
---
We move the block metadata querying to use listStatus() call which
accepts a directory as input and fetches the 'BlockLocation' objects
for every file recursively in that directory. This improves the
metadata loading in the following ways.

- For non-partitioned tables, we query all the BlockLocations in a
single RPC call in the base table directory and load the corresponding
disk IDs.

- For partitioned tables, we query the BlockLocations for all the
partitions residing under the base table directories in a single RPC
and then load every partition with non-default partition directory
separately.

- REFRESH on a table reloads the block metadata from scratch for
every data file every time. So it can be used as a replacement for
invalidate in situations like HDFS block rebalancing which needs
block metadata update.

Also, this patch does away with VolumeIds returned by the HDFS NN
and uses the new StorageIDs returned by the BlockLocation class.
These StorageIDs are UUID strings and hence are mapped to a
per-node 0-based index as expected by the backend. In the upcoming
versions of Hadoop APIs, getFileBlockStorageLocations() is deprecated
and instead the listStatus() returns BlockLocations with storage IDs
embedded. This patch makes use of this improvement to reduce an
additional RPC to the NN to fetch the storage locations.

Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
---
A fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
5 files changed, 379 insertions(+), 345 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/5148/13
-- 
To view, visit http://gerrit.cloudera.org:8080/5148
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Mostafa Mokhtar

[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading

2016-11-29 Thread Bharath Vissapragada (Code Review)

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
..


Patch Set 12:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/5148/12//COMMIT_MSG
Commit Message:

Line 15: We loop throuh each and every file in the table/partition directories
> through
Done


Line 17: This results in large no. of RPC calls to namenode, especially with
> no. -> number
Done


Line 35: 
> mention the behavior change of REFRESH
Done


http://gerrit.cloudera.org:8080/#/c/5148/12/fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
File fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java:

Line 31:  * - To maintain consistent mapping across all the table instances so 
that the disk thread
> ... a consistent mapping ... so that the assignment of scan ranges to I/O t
Done


http://gerrit.cloudera.org:8080/#/c/5148/12/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:

Line 288:* Queries the filesystem to load the file block metadata (e.g. DFS 
blocks) for the
> Suggest rephrasing:
Done


Line 296:*   and enumerate all its blocks and their corresponding hosts and 
disk IDs.
> remove part about disk ids, I think that's the next point
Done


Line 778: LOG.debug("partsByPath size: " + partsByPath.size());
> check log lvl
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4504: fix races in PlanFragmentExecutor regarding status reporting

2016-11-29 Thread Henry Robinson (Code Review)

Henry Robinson has posted comments on this change.

Change subject: IMPALA-4504: fix races in PlanFragmentExecutor regarding status 
reporting
..


Patch Set 4: Code-Review+1

(7 comments)

http://gerrit.cloudera.org:8080/#/c/5250/4/be/src/runtime/plan-fragment-executor.cc
File be/src/runtime/plan-fragment-executor.cc:

PS4, Line 294: if (!status.ok()) {
 :
nit: one line


PS4, Line 298: opened_promise_.Get()
here it seems clearer to return status.


PS4, Line 331: RETURN_IF_ERROR(opened_promise_.Get());
I'm still of the opinion that this logic is better placed in FragmentExecState, 
e.g.:

  if (fragment->Open().OK()) {
fragment->Exec();
  }
  fragment->Close();

and then here you could just DCHECK(opened_promise.IsSet() && 
opened_promise_.Get().OK()); which is a simpler lifecycle invariant. That's in 
keeping with how the PFE lifecycle is managed now, and it seems to make sense 
to have all that logic in one place.

For example, we don't have the same logic in Open() wrt prepare_promise_, which 
we should do for consistency with this method as implemented here.


PS4, Line 447: that
the


Line 489: 
how about DCHECK(!report_thread_active_) here? I'd like to get some guarantee 
that FragmentComplete() was called in all executions it should have been before 
Close().


http://gerrit.cloudera.org:8080/#/c/5250/3/be/src/runtime/plan-fragment-executor.h
File be/src/runtime/plan-fragment-executor.h:

PS3, Line 66: setting that flag to 0 disables periodic reporting altogether
> Isn't that what the next sentence is saying? Or am I misunderstanding your 
Oh yeah. I was thrown off by 'disables periodic reporting altogether', which 
while correct was confusing.


http://gerrit.cloudera.org:8080/#/c/5250/4/be/src/runtime/plan-fragment-executor.h
File be/src/runtime/plan-fragment-executor.h:

PS4, Line 249:  
Nit: extra space.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I052b7b4fabb341ad27ad294cd5b0a53728d87d0e
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading

2016-11-29 Thread Alex Behm (Code Review)

Alex Behm has posted comments on this change.

Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
..

Patch Set 12: Code-Review+1

(7 comments)

I'm pretty happy with the changes. Let's let Dimitris take a look.

http://gerrit.cloudera.org:8080/#/c/5148/12//COMMIT_MSG
Commit Message:

Line 15: We loop throuh each and every file in the table/partition directories
through

Line 17: This results in large no. of RPC calls to namenode, especially with
no. -> number

to the NameNode

especially for large tables

Line 35:
mention the behavior change of REFRESH

http://gerrit.cloudera.org:8080/#/c/5148/12/fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
File fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java:

Line 31: * - To maintain consistent mapping across all the table instances so
that the disk thread
... a consistent mapping ... so that the assignment of scan ranges to I/O
threads is balanced and consistent for all scans on the same host.

http://gerrit.cloudera.org:8080/#/c/5148/12/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:

Line 288:* Queries the filesystem to load the file block metadata (e.g. DFS
blocks) for the
Suggest rephrasing:

Drops and re-loads the block metadata for all partitions in 'partsByPath' whose
location is under the given 'dirPath'. It involves the following steps:
...

Line 296:* and enumerate all its blocks and their corresponding hosts and
disk IDs.
remove part about disk ids, I think that's the next point

Line 778: LOG.debug("partsByPath size: " + partsByPath.size());
check log lvl

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Mostafa Mokhtar
Gerrit-HasComments: Yes

83 matches

Mail list logo