[Impala-ASF-CR] IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

2016-08-17 Thread Sailesh Mukil (Code Review)
Sailesh Mukil has uploaded a new change for review.

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

Change subject: IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot 
fail
..

IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

HdfsTableSink::Close() makes calls to functions that can fail with a
Status. However, since the function has a void return type, these
error statuses are just logged and we cannot take any action according
to the type of error.

This patch moves the closing of the partition file into the FlushFinal
function from Close(), so that in case of an error on closing the
file, the error is propagated up and some action can be taken.

We try and close all the partition files in the map in Close() as well
because if a query is cancelled, FlushFinal will not be called and we
would end up leaking some file descriptors.

Also fixed some long lines in this patch.

Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
---
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
2 files changed, 22 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/4018/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4018
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 


[Impala-ASF-CR] IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

2016-08-17 Thread Sailesh Mukil (Code Review)
Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot 
fail
..


Patch Set 1:

> Uploaded patch set 1.

Contingent on success of the following private build:
http://sandbox.jenkins.cloudera.com/view/Impala/view/Private-Utility/job/impala-private-build-and-test/3969/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

2016-08-18 Thread Sailesh Mukil (Code Review)
Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot 
fail
..


Patch Set 1:

> > Uploaded patch set 1.
 > 
 > Contingent on success of the following private build:
 > http://sandbox.jenkins.cloudera.com/view/Impala/view/Private-Utility/job/impala-private-build-and-test/3969/

This failed some tests, so will fix and post another patch.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

2016-08-18 Thread Dan Hecht (Code Review)
Dan Hecht has posted comments on this change.

Change subject: IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot 
fail
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4018/1/be/src/exec/hdfs-table-sink.cc
File be/src/exec/hdfs-table-sink.cc:

Line 648: if (!finalize_status.ok()) {
MergeStatus() does this check for you.
But why do we have to do this if Close() will go ahead and loop through all 
files and close any that were left open?


Line 653: }
why can't this stay in Close() so that it can more easily always happen, given 
that it doesn't have an error to propagate?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

2016-08-18 Thread Sailesh Mukil (Code Review)
Sailesh Mukil has uploaded a new patch set (#2).

Change subject: IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot 
fail
..

IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

HdfsTableSink::Close() makes calls to functions that can fail with a
Status. However, since the function has a void return type, these
error statuses are just logged and we cannot take any action according
to the type of error.

This patch moves the closing of the partition file into the FlushFinal
function from Close(), so that in case of an error on closing the
file, the error is propagated up and some action can be taken.

We try and close all the partition files in the map in Close() as well
because if a query is cancelled, FlushFinal will not be called and we
would end up leaking some file descriptors.

Also fixed some long lines in this patch.

Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
---
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
2 files changed, 16 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/4018/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4018
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 


[Impala-ASF-CR] IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

2016-08-18 Thread Sailesh Mukil (Code Review)
Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot 
fail
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4018/1/be/src/exec/hdfs-table-sink.cc
File be/src/exec/hdfs-table-sink.cc:

Line 648:   }
> MergeStatus() does this check for you.
I misunderstood the working of the ClosePartitionFile() function. So I removed 
the erase from the map now as it wouldn't save us too much.


Line 653: void HdfsTableSink::Close(RuntimeState* state) {
> why can't this stay in Close() so that it can more easily always happen, gi
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

2016-08-18 Thread Dan Hecht (Code Review)
Dan Hecht has posted comments on this change.

Change subject: IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot 
fail
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4018/2/be/src/exec/hdfs-table-sink.cc
File be/src/exec/hdfs-table-sink.cc:

Line 647: flush_status.MergeStatus(FinalizePartitionFile(state, 
cur_partition->second.first));
I still don't see why this is MergeStatus() instead of RETURN_IF_ERROR.  We 
have to require that Close() can cleanup whatever state we leave things in if 
FlushFinal() fails, so it doesn't seem like we need to continue here.  After 
all, we may have returned early at e.g. line 638 and not even gotten into this 
loop.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

2016-08-19 Thread Sailesh Mukil (Code Review)
Sailesh Mukil has uploaded a new patch set (#3).

Change subject: IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot 
fail
..

IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

HdfsTableSink::Close() makes calls to functions that can fail with a
Status. However, since the function has a void return type, these
error statuses are just logged and we cannot take any action according
to the type of error.

This patch moves the closing of the partition file into the FlushFinal
function from Close(), so that in case of an error on closing the
file, the error is propagated up and some action can be taken.

We try and close all the partition files in the map in Close() as well
because if a query is cancelled, FlushFinal will not be called and we
would end up leaking some file descriptors.

Also fixed some long lines in this patch.

Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
---
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
2 files changed, 14 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/4018/3
-- 
To view, visit http://gerrit.cloudera.org:8080/4018
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 


[Impala-ASF-CR] IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

2016-08-19 Thread Sailesh Mukil (Code Review)
Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot 
fail
..


Patch Set 3:

(1 comment)

Passed private build:
http://sandbox.jenkins.cloudera.com/job/impala-private-build-and-test/3984/

http://gerrit.cloudera.org:8080/#/c/4018/2/be/src/exec/hdfs-table-sink.cc
File be/src/exec/hdfs-table-sink.cc:

Line 647: RETURN_IF_ERROR(FinalizePartitionFile(state, 
cur_partition->second.first));
> I still don't see why this is MergeStatus() instead of RETURN_IF_ERROR.  We
Right, that makes sense. I've made that change.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

2016-08-19 Thread Sailesh Mukil (Code Review)
Sailesh Mukil has uploaded a new patch set (#4).

Change subject: IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot 
fail
..

IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

HdfsTableSink::Close() makes calls to functions that can fail with a
Status. However, since the function has a void return type, these
error statuses are just logged and we cannot take any action according
to the type of error.

This patch moves the closing of the partition file into the FlushFinal
function from Close(), so that in case of an error on closing the
file, the error is propagated up and some action can be taken.

We try and close all the partition files in the map in Close() as well
because if a query is cancelled, FlushFinal will not be called and we
would end up leaking some file descriptors.

Also fixed some long lines in this patch.

Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
---
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
2 files changed, 13 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/4018/4
-- 
To view, visit http://gerrit.cloudera.org:8080/4018
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 


[Impala-ASF-CR] IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

2016-08-19 Thread Dan Hecht (Code Review)
Dan Hecht has posted comments on this change.

Change subject: IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot 
fail
..


Patch Set 4:

Does this pass the tests?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

2016-08-19 Thread Sailesh Mukil (Code Review)
Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot 
fail
..


Patch Set 4:

> Does this pass the tests?

Yes, I mentioned about that it passed the private build:
http://sandbox.jenkins.cloudera.com/job/impala-private-build-and-test/3984/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

2016-08-19 Thread Dan Hecht (Code Review)
Dan Hecht has posted comments on this change.

Change subject: IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot 
fail
..


Patch Set 4: Code-Review+2

(1 comment)

Oops, sorry missed your comment.

http://gerrit.cloudera.org:8080/#/c/4018/4/be/src/exec/hdfs-table-sink.cc
File be/src/exec/hdfs-table-sink.cc:

Line 626:   ImpaladMetrics::NUM_FILES_OPEN_FOR_INSERT->Increment(-1);
Should we do these before returning error? otherwise, the impalad metric can 
get stuck at non-zero, but there's nothing we can do about it right?


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

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


[Impala-ASF-CR] IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

2016-08-19 Thread Sailesh Mukil (Code Review)
Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot 
fail
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4018/4/be/src/exec/hdfs-table-sink.cc
File be/src/exec/hdfs-table-sink.cc:

Line 626:   ImpaladMetrics::NUM_FILES_OPEN_FOR_INSERT->Increment(-1);
> Should we do these before returning error? otherwise, the impalad metric ca
Yes, we would want that to not be decremented in case of an error.
So that test_verify_metrics can catch that a file(s) was not closed, in case of 
our test runs.

And otherwise too, users can see that there are some files still open.


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

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


[Impala-ASF-CR] IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

2016-08-19 Thread Dan Hecht (Code Review)
Dan Hecht has posted comments on this change.

Change subject: IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot 
fail
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4018/4/be/src/exec/hdfs-table-sink.cc
File be/src/exec/hdfs-table-sink.cc:

Line 626:   ImpaladMetrics::NUM_FILES_OPEN_FOR_INSERT->Increment(-1);
> Yes, we would want that to not be decremented in case of an error.
Hmm, but what can a user do about that? and does an error mean the file isn't 
closed or that there is something else wrong with the file (maybe not even 
open).


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

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


[Impala-ASF-CR] IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

2016-08-19 Thread Sailesh Mukil (Code Review)
Hello Dan Hecht,

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

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

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

Change subject: IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot 
fail
..

IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

HdfsTableSink::Close() makes calls to functions that can fail with a
Status. However, since the function has a void return type, these
error statuses are just logged and we cannot take any action according
to the type of error.

This patch moves the closing of the partition file into the FlushFinal
function from Close(), so that in case of an error on closing the
file, the error is propagated up and some action can be taken.

We try and close all the partition files in the map in Close() as well
because if a query is cancelled, FlushFinal will not be called and we
would end up leaking some file descriptors.

Also fixed some long lines in this patch.

Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
---
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
2 files changed, 15 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/4018/5
-- 
To view, visit http://gerrit.cloudera.org:8080/4018
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 


[Impala-ASF-CR] IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

2016-08-19 Thread Sailesh Mukil (Code Review)
Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot 
fail
..


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4018/4/be/src/exec/hdfs-table-sink.cc
File be/src/exec/hdfs-table-sink.cc:

Line 626:   }
> Hmm, but what can a user do about that? and does an error mean the file isn
As we spoke, this metric is not to catch errors, but just to count the number 
of tries to open and close files.


http://gerrit.cloudera.org:8080/#/c/4018/5/be/src/exec/hdfs-table-sink.cc
File be/src/exec/hdfs-table-sink.cc:

PS5, Line 620: partition->tmp_hdfs_file = NULL;
Had to move this up too. Else, we could have cases where the metric is 
decremented twice for the same file.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

2016-08-19 Thread Dan Hecht (Code Review)
Dan Hecht has posted comments on this change.

Change subject: IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot 
fail
..


Patch Set 5: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

2016-08-19 Thread Internal Jenkins (Code Review)
Internal Jenkins has posted comments on this change.

Change subject: IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot 
fail
..


Patch Set 5: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

2016-08-19 Thread Internal Jenkins (Code Review)
Internal Jenkins has submitted this change and it was merged.

Change subject: IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot 
fail
..


IMPALA-2988: Refactor HdfsTableSink::Close() so that it cannot fail

HdfsTableSink::Close() makes calls to functions that can fail with a
Status. However, since the function has a void return type, these
error statuses are just logged and we cannot take any action according
to the type of error.

This patch moves the closing of the partition file into the FlushFinal
function from Close(), so that in case of an error on closing the
file, the error is propagated up and some action can be taken.

We try and close all the partition files in the map in Close() as well
because if a query is cancelled, FlushFinal will not be called and we
would end up leaking some file descriptors.

Also fixed some long lines in this patch.

Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
Reviewed-on: http://gerrit.cloudera.org:8080/4018
Reviewed-by: Dan Hecht 
Tested-by: Internal Jenkins
---
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
2 files changed, 15 insertions(+), 13 deletions(-)

Approvals:
  Internal Jenkins: Verified
  Dan Hecht: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I2546bc68ba136b2713d744c1b920878606a2217b
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Sailesh Mukil