You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@impala.apache.org by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org> on 2020/05/19 12:47:56 UTC

[Impala-ASF-CR] IMPALA-9764: TransactionKeepalive should set sleep interval based on Hive Configuration

Zoltan Borok-Nagy has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15956

Change subject: IMPALA-9764: TransactionKeepalive should set sleep interval based on Hive Configuration
......................................................................

IMPALA-9764: TransactionKeepalive should set sleep interval based on Hive Configuration

Impala must heartbeat its transactions and locks in order to keep them
alive. Before this commit TransactionKeepalive had hard-coded value for
sleep interval, i.e. it slept 60 seconds between heartbeats.

However, at HMS side the transaction timeout is configurable.

With this commit Impala reads the Hive configuration and calculate its
sleep interval based on that. It simply divides the timeout value by 3.

Testing:

I tested it manually. Created hive-site.xml with "hive.txn.timeout" set
and checked that Impala picked up the set value. If "hive.txn.timeout"
was not set, Impala used the default value (300s).

Change-Id: I546dac324494f1c945e8943e267f8146d22a837d
---
M fe/src/main/java/org/apache/impala/common/TransactionKeepalive.java
1 file changed, 13 insertions(+), 7 deletions(-)

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I546dac324494f1c945e8943e267f8146d22a837d
Gerrit-Change-Number: 15956
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-9764: TransactionKeepalive should set sleep interval based on Hive Configuration

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15956 )

Change subject: IMPALA-9764: TransactionKeepalive should set sleep interval based on Hive Configuration
......................................................................


Patch Set 2: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I546dac324494f1c945e8943e267f8146d22a837d
Gerrit-Change-Number: 15956
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 19 May 2020 20:56:14 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9764: TransactionKeepalive should set sleep interval based on Hive Configuration

Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.

Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/15956 )

Change subject: IMPALA-9764: TransactionKeepalive should set sleep interval based on Hive Configuration
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/15956/1/fe/src/main/java/org/apache/impala/common/TransactionKeepalive.java
File fe/src/main/java/org/apache/impala/common/TransactionKeepalive.java:

http://gerrit.cloudera.org:8080/#/c/15956/1/fe/src/main/java/org/apache/impala/common/TransactionKeepalive.java@211
PS1, Line 211:     HiveConf hiveConf = new HiveConf(TransactionKeepalive.class);
> Can we always assume that there will be a hive-site.xml on the machine? Wha
At least in my dev environment Impala won't start if it doesn't find a hive-site.xml.

If the configuration value is missing from the hive-site.xml then the default value is used.

Thanks for mentioning the HMS API. I gave it a try, it requires a bit more boilerplate code, but the main problem with it is that it can throw exceptions, therefore I'd need to update the throws clauses at several places. Since we are already using HiveConf at other places I think it's safe to depend on it here as well.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I546dac324494f1c945e8943e267f8146d22a837d
Gerrit-Change-Number: 15956
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 19 May 2020 15:31:25 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9764: TransactionKeepalive should set sleep interval based on Hive Configuration

Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.

Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/15956 )

Change subject: IMPALA-9764: TransactionKeepalive should set sleep interval based on Hive Configuration
......................................................................


Patch Set 1: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/15956/1/fe/src/main/java/org/apache/impala/common/TransactionKeepalive.java
File fe/src/main/java/org/apache/impala/common/TransactionKeepalive.java:

http://gerrit.cloudera.org:8080/#/c/15956/1/fe/src/main/java/org/apache/impala/common/TransactionKeepalive.java@211
PS1, Line 211:     HiveConf hiveConf = new HiveConf(TransactionKeepalive.class);
Can we always assume that there will be a hive-site.xml on the machine? What will happen if there isn't any, will we use the default values?

An alternative could be to ask HMS about the configs:
https://github.com/apache/hive/blob/037eacea46371015a7f9894c5a9ccfb9708d5c56/standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift#L2298

I see that we already create a new HiveConfg in other parts of Impala, so I am ok with keeping this pattern in the is patch, just wanted to share my doubts about our approach.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I546dac324494f1c945e8943e267f8146d22a837d
Gerrit-Change-Number: 15956
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 19 May 2020 13:40:42 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9764: TransactionKeepalive should set sleep interval based on Hive Configuration

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15956 )

Change subject: IMPALA-9764: TransactionKeepalive should set sleep interval based on Hive Configuration
......................................................................

IMPALA-9764: TransactionKeepalive should set sleep interval based on Hive Configuration

However, at HMS side the transaction timeout is configurable.

With this commit Impala reads the Hive configuration and calculate its
sleep interval based on that. It simply divides the timeout value by 3.

Testing:

I tested it manually. Created hive-site.xml with "hive.txn.timeout" set
and checked that Impala picked up the set value. If "hive.txn.timeout"
was not set, Impala used the default value (300s).

Change-Id: I546dac324494f1c945e8943e267f8146d22a837d
Reviewed-on: http://gerrit.cloudera.org:8080/15956
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/common/TransactionKeepalive.java
1 file changed, 13 insertions(+), 7 deletions(-)

Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I546dac324494f1c945e8943e267f8146d22a837d
Gerrit-Change-Number: 15956
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>

[Impala-ASF-CR] IMPALA-9764: TransactionKeepalive should set sleep interval based on Hive Configuration

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15956 )

Change subject: IMPALA-9764: TransactionKeepalive should set sleep interval based on Hive Configuration
......................................................................


Patch Set 1:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/6099/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I546dac324494f1c945e8943e267f8146d22a837d
Gerrit-Change-Number: 15956
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 19 May 2020 13:41:11 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9764: TransactionKeepalive should set sleep interval based on Hive Configuration

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15956 )

Change subject: IMPALA-9764: TransactionKeepalive should set sleep interval based on Hive Configuration
......................................................................


Patch Set 2:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5858/ DRY_RUN=false


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I546dac324494f1c945e8943e267f8146d22a837d
Gerrit-Change-Number: 15956
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 19 May 2020 15:31:53 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9764: TransactionKeepalive should set sleep interval based on Hive Configuration

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15956 )

Change subject: IMPALA-9764: TransactionKeepalive should set sleep interval based on Hive Configuration
......................................................................


Patch Set 2: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I546dac324494f1c945e8943e267f8146d22a837d
Gerrit-Change-Number: 15956
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 19 May 2020 15:31:52 +0000
Gerrit-HasComments: No