-
Notifications
You must be signed in to change notification settings - Fork 230
kcl github workflow #336
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
kcl github workflow #336
Conversation
|
Still reviewing, but let's make sure we merge with a more descriptive commit message. Something like |
| fi | ||
|
|
||
| echo "==========ERROR LOGS==========" | ||
| grep -i error kcl_output.log || echo "No errors found in logs" No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonder if there's anything else we would care about besides "error" here, like "fatal", "failed" or something even more general like "exception". Don't think its needed for now and we can always look at failed runs in the future and add stuff to this grep if we find ones that time out but don't show error logs.
| echo "Table deletion failed, attempt $i/10. Retrying DynamoDB Table deletion in $((i * 3)) seconds" && sleep $((i * 3)) | ||
| done | ||
| for SUFFIX in "-CoordinatorState" "-WorkerMetricStats"; do | ||
| if aws dynamodb describe-table --table-name $APP_NAME$SUFFIX &>/dev/null; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to describe on these tables but not the main lease table?
| for i in {1..10}; do | ||
| echo "Deleting table $APP_NAME" | ||
| aws dynamodb delete-table --table-name $APP_NAME && break || | ||
| echo "Table deletion failed, attempt $i/10. Retrying DynamoDB Table deletion in $((i * 3)) seconds" && sleep $((i * 3)) | ||
| done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommend making a bash function like
delete_table()
table_name=$1
if aws dynamodb describe-table --table-name $table_name &>/dev/null; then
echo "Deleting table $table_name"
# Do table deletion retries here
else
echo "Table $table_name does not exist and does not need to be cleaned up"
fi
So the primary script can just make an array with the three KCL tables and call the function to reduce the code duplication
| #!/bin/bash | ||
| set -e | ||
|
|
||
| # Manipulate sample.properties file that the KCL application pulls properties from (ex: streamName, applicationName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we ever change the sample properties, some of this may not work anymore right? I see there are some assumptions of what the sample properties should be and some explicit find and replace. Feels finicky and can be easily broken in the future if for example we add a new property. Would it be easier to just create a folder which contains the properties files we are going to use for the github actions and have the workflow use that file?
| # Get records from stream to verify they exist before continuing | ||
| SHARD_ITERATOR=$(aws kinesis get-shard-iterator --stream-name $STREAM_NAME --shard-id shardId-000000000000 --shard-iterator-type TRIM_HORIZON --query 'ShardIterator' --output text) | ||
| INITIAL_RECORDS=$(aws kinesis get-records --shard-iterator $SHARD_ITERATOR) | ||
| RECORD_COUNT_BEFORE=$(echo $INITIAL_RECORDS | jq '.Records | length') | ||
|
|
||
| echo "Found $RECORD_COUNT_BEFORE records in stream before KCL start" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we do this a second time? I see this is also done in put_words_stream.sh
Issue #, if available:
Description of changes:
Adds new KCL github workflow
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.