Skip to content
This repository was archived by the owner on May 12, 2021. It is now read-only.

Conversation

CyberQuestor
Copy link
Contributor

All,

I have discovered a potential improvement for redeploy-script (predictionio/examples/redeploy-script/redeploy.sh). Details are outlined below.

PIO information

  • Version of PredictionIO - apache-predictionio-0.13.0
  • Other components used
    • hadoop-2.8.2
    • spark-2.1.2
    • elasticsearch-5.6.4
    • hbase-1.3.2
    • openjdk-8-jdk

Issue description

The redeploy script provided in examples is crucial for production deployments to fully leverage real-time ingestion of events.

However, after extended usage I noticed that whilst pio deploy within script successfully undeploys any existing engine instance and binds a new one to the same port; it fails to clear up resources allocated to previous deployment (read as PID continues to linger). Continued usage will lead to lack of memory forcing us to kill stale processes manually.

Code example

# Deploy
DEPLOY_LOG=`mktemp $LOG_DIR/tmp.XXXXXXXXXX`
$($DEPLOY_COMMAND 1>$DEPLOY_LOG 2>&1) &

# Check if the engine is up
sleep 60
curl $HOSTNAME:$PORT > /dev/null
RETURN_VAL=$?
COUNTER=0
while [[ $RETURN_VAL -ne 0 && $COUNTER -lt 20 ]]; do
  sleep 30
  curl $HOSTNAME:$PORT > /dev/null
  let RETURN_VAL=$?
  let COUNTER=COUNTER+1
done

Expected behaviour

  • Deploys new engine instance
  • Eliminates old process

Actual results

  • Deploys new engine instance
  • Retains old process

Suggested resolution

This can be easily handled by;

  • Looking for PID of any existing engine instance
  • Complete current pio deploy cycle
  • If PID for previous instance exist, kill process thus releasing resources.
# Deploy
# Get current running instance PID
PIDBYPORT_COMMAND="lsof -t -i:$PORT"
DEPLOYEDPORT=$($PIDBYPORT_COMMAND)

DEPLOY_LOG=`mktemp $LOG_DIR/tmp.XXXXXXXXXX`
$($DEPLOY_COMMAND 1>$DEPLOY_LOG 2>&1) &

# Check if the engine is up
sleep 60
curl $HOSTNAME:$PORT > /dev/null
RETURN_VAL=$?
COUNTER=0
while [[ $RETURN_VAL -ne 0 && $COUNTER -lt 20 ]]; do
  sleep 30
  curl $HOSTNAME:$PORT > /dev/null
  let RETURN_VAL=$?
  let COUNTER=COUNTER+1
done

# Check if the previous engine instance is running
KILLSD_COMMAND="kill $DEPLOYEDPORT"
if [ -z "$DEPLOYEDPORT" ]
then
  printf "\nNo stale PIDs found for port $PORT\n"
else
  $($KILLSD_COMMAND)
  printf "\nStale PID found as $DEPLOYEDPORT. Resources released.\n"
fi

This pull request has suggested improvement.

# Deploy
# Get current running instance PID
PIDBYPORT_COMMAND="lsof -t -i:$PORT"
DEPLOYEDPORT=$($PIDBYPORT_COMMAND)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using DEPLOYEDPORT, I think maybe DEPLOYEDPID is more accurate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right @Wei-1 . That would be more accurate in context. Updated.

@shimamoto
Copy link
Member

@tom-chan @dszeto Do you have time to review this PR?

@EmergentOrder EmergentOrder self-requested a review November 5, 2019 15:31
Copy link
Member

@EmergentOrder EmergentOrder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@EmergentOrder EmergentOrder merged commit d6bae3a into apache:develop Nov 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants