You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Pranay Singh (Code Review)" <ge...@cloudera.org> on 2017/08/17 20:33:08 UTC

[Impala-ASF-CR] IMPALA-5108:idle session timeout kicks in later than expected 

Pranay Singh has uploaded a new change for review.

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

Change subject: IMPALA-5108:idle_session_timeout kicks in later than expected
......................................................................

IMPALA-5108:idle_session_timeout kicks in later than expected

Fix: The issue was caused because polling frequency to check for
the session to be expired was half of session timeout, which is
around ~1 min. The polling frequency has been increased, it's changed
to be around 1 sec with this change.

In addition to the above change, the time that displays the Expiry
time of session is displayed in local time zone, this is to better
co-relate with the time detail in the logs.

The test changes in session-expiry-test is based on introducing
variable names for the numeric constants and to reduce the time of
max_idle_timeout_ms.

Change-Id: Ic62f1ece6c128a80676c6eb331ef190a038bee76
---
M be/src/service/impala-server.cc
M be/src/service/session-expiry-test.cc
2 files changed, 14 insertions(+), 8 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic62f1ece6c128a80676c6eb331ef190a038bee76
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh

[Impala-ASF-CR] IMPALA-5108:idle session timeout kicks in later than expected

Posted by "Pranay Singh (Code Review)" <ge...@cloudera.org>.
Pranay Singh has uploaded a new patch set (#2).

Change subject: IMPALA-5108:idle_session_timeout kicks in later than expected
......................................................................

IMPALA-5108:idle_session_timeout kicks in later than expected

Fix: The issue was caused because polling frequency to check for
    the session to be expired was half of session timeout, which is
    around ~1 min. The polling frequency has been increased, it's changed
    to be around 1 sec with this change.

    The test changes in session-expiry-test is based on introducing
    variable names for the numeric constants and to reduce the time of
    max_idle_timeout_ms.

Change-Id: Ic62f1ece6c128a80676c6eb331ef190a038bee76
---
M be/src/service/impala-server.cc
M be/src/service/session-expiry-test.cc
2 files changed, 10 insertions(+), 8 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic62f1ece6c128a80676c6eb331ef190a038bee76
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Pranay Singh

[Impala-ASF-CR] IMPALA-5108: idle session timeout kicks in later than expected

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5108: idle_session_timeout kicks in later than expected
......................................................................


Patch Set 3: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic62f1ece6c128a80676c6eb331ef190a038bee76
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Pranay Singh
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5108: idle session timeout kicks in later than expected

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has submitted this change and it was merged.

Change subject: IMPALA-5108: idle_session_timeout kicks in later than expected
......................................................................


IMPALA-5108: idle_session_timeout kicks in later than expected

Fix: The issue was caused because sessions could get closed
     some time after their timeout expires, because the session
     expiration thread only woke up every session-timeout/2
     seconds. This fix changes the logic, now the session expiration
     thread will wakeup every 1 sec to check whether sessions can
     be expired.

    The test changes in session-expiry-test is based on introducing
    variable names for the numeric constants and to reduce the time
    of max_idle_timeout_ms.

Change-Id: Ic62f1ece6c128a80676c6eb331ef190a038bee76
Reviewed-on: http://gerrit.cloudera.org:8080/7709
Tested-by: Impala Public Jenkins
Reviewed-by: Henry Robinson <he...@cloudera.com>
---
M be/src/service/impala-server.cc
M be/src/service/session-expiry-test.cc
2 files changed, 9 insertions(+), 8 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ic62f1ece6c128a80676c6eb331ef190a038bee76
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Pranay Singh

[Impala-ASF-CR] IMPALA-5108: idle session timeout kicks in later than expected

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.

Change subject: IMPALA-5108: idle_session_timeout kicks in later than expected
......................................................................


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic62f1ece6c128a80676c6eb331ef190a038bee76
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Pranay Singh
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5108: idle session timeout kicks in later than expected

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5108: idle_session_timeout kicks in later than expected
......................................................................


Patch Set 3:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1098/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic62f1ece6c128a80676c6eb331ef190a038bee76
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Pranay Singh
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5108: idle session timeout kicks in later than expected

Posted by "Pranay Singh (Code Review)" <ge...@cloudera.org>.
Pranay Singh has uploaded a new patch set (#3).

Change subject: IMPALA-5108: idle_session_timeout kicks in later than expected
......................................................................

IMPALA-5108: idle_session_timeout kicks in later than expected

Fix: The issue was caused because sessions could get closed
     some time after their timeout expires, because the session
     expiration thread only woke up every session-timeout/2
     seconds. This fix changes the logic, now the session expiration
     thread will wakeup every 1 sec to check whether sessions can
     be expired.

    The test changes in session-expiry-test is based on introducing
    variable names for the numeric constants and to reduce the time
    of max_idle_timeout_ms.

Change-Id: Ic62f1ece6c128a80676c6eb331ef190a038bee76
---
M be/src/service/impala-server.cc
M be/src/service/session-expiry-test.cc
2 files changed, 9 insertions(+), 8 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic62f1ece6c128a80676c6eb331ef190a038bee76
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Pranay Singh

[Impala-ASF-CR] IMPALA-5108:idle session timeout kicks in later than expected

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.

Change subject: IMPALA-5108:idle_session_timeout kicks in later than expected
......................................................................


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/7709/1/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

PS1, Line 1817:   
double space


PS1, Line 1817: that
whether


PS1, Line 1819: int64_t session_timeout_min_ms = 1000;
Rename this to something like sleep_time_ms , and make it a constant - e.g.:

  const int64_t SLEEP_TIME_MS = 1000;


PS1, Line 1844: FLAGS_use_local_tz_for_unix_timestamp_conversions = true;
This is very dangerous: what if some other thread reads FLAGS_use_local_tz_for_unix_timestamp_conversions concurrently?

I'd leave this change out for now.


http://gerrit.cloudera.org:8080/#/c/7709/1/be/src/service/session-expiry-test.cc
File be/src/service/session-expiry-test.cc:

PS1, Line 45: num_clients
constants should be ALL_CAPS


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic62f1ece6c128a80676c6eb331ef190a038bee76
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5108:idle session timeout kicks in later than expected

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.

Change subject: IMPALA-5108:idle_session_timeout kicks in later than expected
......................................................................


Patch Set 2: Code-Review+2

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7709/2//COMMIT_MSG
Commit Message:

Line 7: IMPALA-5108:idle_session_timeout kicks in later than expected
nit: put a space after ':'


PS2, Line 11: ~1 min
that's only a default - would be clearer to the reader to say "Sessions could get closed some time after their timeout expires, because the session expiration thread only woke up every session-timeout/2 seconds."


http://gerrit.cloudera.org:8080/#/c/7709/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

PS2, Line 1817: // Sleep for a second before checking whether an active session
              :         // can be expired or not.
nit: can you wrap this to 90 chars?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic62f1ece6c128a80676c6eb331ef190a038bee76
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Pranay Singh
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5108:idle session timeout kicks in later than expected

Posted by "Pranay Singh (Code Review)" <ge...@cloudera.org>.
Pranay Singh has posted comments on this change.

Change subject: IMPALA-5108:idle_session_timeout kicks in later than expected
......................................................................


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/7709/1/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

PS1, Line 1817: that
> whether
that replaced by whether, done


PS1, Line 1817:   
> double space
Double space Removed


PS1, Line 1819: int64_t session_timeout_min_ms = 1000;
> Rename this to something like sleep_time_ms , and make it a constant - e.g.
Made it a constant and changed it to uppercase


PS1, Line 1844: FLAGS_use_local_tz_for_unix_timestamp_conversions = true;
> This is very dangerous: what if some other thread reads FLAGS_use_local_tz_
Removed it


http://gerrit.cloudera.org:8080/#/c/7709/1/be/src/service/session-expiry-test.cc
File be/src/service/session-expiry-test.cc:

PS1, Line 45: num_clients
> constants should be ALL_CAPS
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic62f1ece6c128a80676c6eb331ef190a038bee76
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Pranay Singh
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5108: idle session timeout kicks in later than expected

Posted by "Pranay Singh (Code Review)" <ge...@cloudera.org>.
Hello Henry Robinson,

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

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

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

Change subject: IMPALA-5108: idle_session_timeout kicks in later than expected
......................................................................

IMPALA-5108: idle_session_timeout kicks in later than expected

Fix: The issue was caused because sessions could get closed
     some time after their timeout expires, because the session
     expiration thread only woke up every session-timeout/2
     seconds. This fix changes the logic, now the session expiration
     thread will wakeup every 1 sec to check whether sessions can
     be expired.

    The test changes in session-expiry-test is based on introducing
    variable names for the numeric constants and to reduce the time
    of max_idle_timeout_ms.

Change-Id: Ic62f1ece6c128a80676c6eb331ef190a038bee76
---
M be/src/service/impala-server.cc
M be/src/service/session-expiry-test.cc
2 files changed, 9 insertions(+), 8 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic62f1ece6c128a80676c6eb331ef190a038bee76
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Pranay Singh

[Impala-ASF-CR] IMPALA-5108:idle session timeout kicks in later than expected

Posted by "Pranay Singh (Code Review)" <ge...@cloudera.org>.
Pranay Singh has uploaded a new patch set (#2).

Change subject: IMPALA-5108:idle_session_timeout kicks in later than expected
......................................................................

IMPALA-5108:idle_session_timeout kicks in later than expected

Fix: The issue was caused because polling frequency to check for
    the session to be expired was half of session timeout, which is
    around ~1 min. The polling frequency has been increased, it's changed
    to be around 1 sec with this change.

    The test changes in session-expiry-test is based on introducing
    variable names for the numeric constants and to reduce the time of
    max_idle_timeout_ms.

Change-Id: Ic62f1ece6c128a80676c6eb331ef190a038bee76
---
M be/src/service/impala-server.cc
M be/src/service/session-expiry-test.cc
2 files changed, 10 insertions(+), 8 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic62f1ece6c128a80676c6eb331ef190a038bee76
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Pranay Singh