You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@samza.apache.org by GitBox <gi...@apache.org> on 2020/12/05 01:39:32 UTC
[GitHub] [samza] lakshmi-manasa-g opened a new pull request #1452: SAMZA-2611: [AM-HA] heartbeat reestablish causes container's heartbeat thread to die
lakshmi-manasa-g opened a new pull request #1452:
URL: https://github.com/apache/samza/pull/1452
Symptom: When new AM takes a long time to to start up, already running container's heartbeat thread silently dies and does not make any heartbeat requests to the new AM.
Cause: AM url (yarn.am.tracking.url) key-value is removed from Coordinator stream when new AM is starting up - as this config is present in old config (aka coordinator stream) but not in the new AM generated config. This causes the running container to fetch a null when its constantly fetching value for this key and thus throws NPE.
Changes: When AMHA is enabled, do not remove this config
Tests: works with hello-samza. Trying to write a unit test but CoordinatorStreamUtil is tricky to mock and inject stuff.
Usage Instructions: N/A
Upgrade Instructions: N/A
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] mynameborat removed a comment on pull request #1452: SAMZA-2611: [AM-HA] heartbeat reestablish causes container's heartbeat thread to die
Posted by GitBox <gi...@apache.org>.
mynameborat removed a comment on pull request #1452:
URL: https://github.com/apache/samza/pull/1452#issuecomment-740003171
Symptom: When new AM takes a long time to to start up, already running container's heartbeat thread silently dies and does not make any heartbeat requests to the new AM.
Cause: AM url (yarn.am.tracking.url) key-value is removed from Coordinator stream when new AM is starting up - as this config is present in old config (aka coordinator stream) but not in the new AM generated config. This causes the running container to fetch a null when its constantly fetching value for this key and thus throws NPE.
Changes: When AMHA is enabled, do not remove this config
Tests: works with hello-samza. Trying to write a unit test but CoordinatorStreamUtil is tricky to mock and inject stuff.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] mynameborat merged pull request #1452: SAMZA-2611: [AM-HA] heartbeat reestablish causes container's heartbeat thread to die
Posted by GitBox <gi...@apache.org>.
mynameborat merged pull request #1452:
URL: https://github.com/apache/samza/pull/1452
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] mynameborat commented on a change in pull request #1452: SAMZA-2611: [AM-HA] heartbeat reestablish causes container's heartbeat thread to die
Posted by GitBox <gi...@apache.org>.
mynameborat commented on a change in pull request #1452:
URL: https://github.com/apache/samza/pull/1452#discussion_r536478041
##########
File path: samza-core/src/main/java/org/apache/samza/container/ContainerHeartbeatMonitor.java
##########
@@ -92,8 +93,13 @@ public void start() {
if (!response.isAlive()) {
if (isApplicationMasterHighAvailabilityEnabled) {
LOG.warn("Failed to establish connection with {}. Checking for new AM", coordinatorUrl);
- if (checkAndEstablishConnectionWithNewAM()) {
- return;
+ try {
+ if (checkAndEstablishConnectionWithNewAM()) {
+ return;
+ }
+ } catch (Exception e) {
+ LOG.warn("Exception trying to connect with new AM", e);
Review comment:
Should be a `LOG.error` as this is fatal and should be treat as such.
##########
File path: samza-core/src/main/java/org/apache/samza/container/ContainerHeartbeatMonitor.java
##########
@@ -92,8 +93,13 @@ public void start() {
if (!response.isAlive()) {
if (isApplicationMasterHighAvailabilityEnabled) {
LOG.warn("Failed to establish connection with {}. Checking for new AM", coordinatorUrl);
- if (checkAndEstablishConnectionWithNewAM()) {
- return;
+ try {
+ if (checkAndEstablishConnectionWithNewAM()) {
+ return;
+ }
+ } catch (Exception e) {
+ LOG.warn("Exception trying to connect with new AM", e);
+ throw new SamzaException(e);
Review comment:
It will kill the thread and recreate one. I'd suggest instead to proceed with exit code as below except log the error of the exception too.
Recreating the thread doesn't guarantee that the subsequent thread can recover from the previous error and potentially run into this in a loop of some sorts without any concrete action.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] mynameborat commented on pull request #1452: SAMZA-2611: [AM-HA] heartbeat reestablish causes container's heartbeat thread to die
Posted by GitBox <gi...@apache.org>.
mynameborat commented on pull request #1452:
URL: https://github.com/apache/samza/pull/1452#issuecomment-740003171
Symptom: When new AM takes a long time to to start up, already running container's heartbeat thread silently dies and does not make any heartbeat requests to the new AM.
Cause: AM url (yarn.am.tracking.url) key-value is removed from Coordinator stream when new AM is starting up - as this config is present in old config (aka coordinator stream) but not in the new AM generated config. This causes the running container to fetch a null when its constantly fetching value for this key and thus throws NPE.
Changes: When AMHA is enabled, do not remove this config
Tests: works with hello-samza. Trying to write a unit test but CoordinatorStreamUtil is tricky to mock and inject stuff.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] mynameborat commented on a change in pull request #1452: SAMZA-2611: [AM-HA] heartbeat reestablish causes container's heartbeat thread to die
Posted by GitBox <gi...@apache.org>.
mynameborat commented on a change in pull request #1452:
URL: https://github.com/apache/samza/pull/1452#discussion_r537610193
##########
File path: samza-core/src/main/java/org/apache/samza/container/ContainerHeartbeatMonitor.java
##########
@@ -92,17 +93,19 @@ public void start() {
if (!response.isAlive()) {
if (isApplicationMasterHighAvailabilityEnabled) {
LOG.warn("Failed to establish connection with {}. Checking for new AM", coordinatorUrl);
- if (checkAndEstablishConnectionWithNewAM()) {
+ try {
+ if (checkAndEstablishConnectionWithNewAM()) {
+ return;
+ }
+ } catch (Exception e) {
+ // On exception in re-establish connection with new AM, force exit.
+ LOG.error("Exception trying to connect with new AM", e);
+ forceExit("failure in establishing cconnection with new AM", 0);
Review comment:
nit: typo `s/cconnection/connection`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org