Skip to content
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

CUMULUS-260 update pdr stats #235

Merged
merged 22 commits into from
Mar 15, 2018
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
e574755
update pdr stats
jennyhliu Mar 6, 2018
c796810
Merge branch 'master' of https://github.com/cumulus-nasa/cumulus into…
jennyhliu Mar 9, 2018
ca5634d
update pdr stats
jennyhliu Mar 9, 2018
ff23e5d
remove update pdr stats from pdr-status-check
jennyhliu Mar 9, 2018
1f3a8ab
use cumulus-message-adapter
jennyhliu Mar 13, 2018
76cad35
Merge branch 'master' into pdrstats
jennyhliu Mar 13, 2018
cd14fd0
update readme
jennyhliu Mar 13, 2018
1601eca
Merge branch 'pdrstats' of https://github.com/cumulus-nasa/cumulus in…
jennyhliu Mar 13, 2018
091e9d4
Merge branch 'master' into pdrstats
Mar 13, 2018
d939030
fixes
jennyhliu Mar 13, 2018
853c835
Merge branch 'pdrstats' of https://github.com/cumulus-nasa/cumulus in…
jennyhliu Mar 13, 2018
0edd3da
pr updates
jennyhliu Mar 14, 2018
f3f3d93
Merge branch 'master' into pdrstats
jennyhliu Mar 14, 2018
a835a3e
Merge branch 'master' into pdrstats
Mar 14, 2018
2ab76f6
Merge branch 'master' into pdrstats
Mar 14, 2018
b2507ce
more fixes
jennyhliu Mar 15, 2018
68bc62e
Merge branch 'pdrstats' of https://github.com/cumulus-nasa/cumulus in…
jennyhliu Mar 15, 2018
4b49dcd
Merge branch 'pdrstats' of https://github.com/cumulus-nasa/cumulus in…
Mar 15, 2018
a9c446d
more fix
jennyhliu Mar 15, 2018
d51ed64
Merge branch 'pdrstats' of https://github.com/cumulus-nasa/cumulus in…
jennyhliu Mar 15, 2018
7307d5f
Merge branch 'master' into pdrstats
jennyhliu Mar 15, 2018
d672146
Merge branch 'master' into pdrstats
jennyhliu Mar 15, 2018
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
- `@cumulus/deployment` deploys DynamoDB streams for the Collections, Providers and Rules tables as well as a new lambda function called `dbIndexer`. The `dbIndexer` lambda has an event source mapping which listens to each of the DynamoDB streams. The dbIndexer lambda receives events referencing operations on the DynamoDB table and updates the elasticsearch cluster accordingly.
- The `@cumulus/api` endpoints for collections, providers and rules _only_ query DynamoDB, with the exception of LIST endpoints and the collections' GET endpoint.

- **CUMULUS-260: "PDR page on dashboard only shows zeros."** The PDR stats in LPDAAC are all 0s, even if the dashboard has been fixed to retrieve the correct fields. The current version of pdr-status-check has a few issues.
- pdr is not included in the input/output schema. It's available from the input event. So the pdr status and stats are not updated when the ParsePdr workflow is complete. Adding the pdr to the input/output of the task will fix this.
- pdr-status-check doesn't update pdr stats which prevent the real time pdr progress from showing up in the dashboard. To solve this, added lambda function sf-sns-report which is copied from @cumulus/api/lambdas/sf-sns-broadcase with modification, sf-sns-report can be used to report step function status anywhere inside a step function. So add step sf-sns-report after each pdr-status-check, we will get the PDR status progress at real time.
Copy link
Contributor

Choose a reason for hiding this comment

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

"broadcast", not "broadcase"

- It's possible a execution is still in the queue and doesn't exist in sfn yet. Added code to handle 'ExecutionDoesNotExist' error when checking the execution status.
Copy link
Contributor

Choose a reason for hiding this comment

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

minor grammar fix: It's possible an execution is still in the queue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


### Updated
- Broke up `kes.override.js` of @cumulus/deployment to multiple modules and moved to a new location
- Expanded @cumulus/deployment test coverage
Expand Down
48 changes: 29 additions & 19 deletions cumulus/tasks/pdr-status-check/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,8 @@ function logStatus(output) {
* failed: [
* { arn: 'arn:456', reason: 'Workflow Aborted' }
* ],
* completed: []
* completed: [],
* pdr: {}
* }
*
* @param {Object} event - the event that came into checkPdrStatuses
Expand Down Expand Up @@ -117,7 +118,8 @@ function buildOutput(event, groupedExecutions) {
isFinished: groupedExecutions.running.length === 0,
running,
failed,
completed
completed,
pdr: event.input.pdr
};

if (!output.isFinished) {
Expand All @@ -137,28 +139,36 @@ function buildOutput(event, groupedExecutions) {
* @returns {Promise.<Object>} - an object describing the status of Step
* Function executions related to a PDR
*/
function checkPdrStatuses(event) {
async function checkPdrStatuses(event) {
const runningExecutionArns = event.input.running || [];

const promisedExecutionDescriptions = runningExecutionArns.map((executionArn) =>
aws.sfn().describeExecution({ executionArn }).promise());

return Promise.all(promisedExecutionDescriptions)
.then(groupExecutionsByStatus)
.then((groupedExecutions) => {
const counter = getCounterFromEvent(event) + 1;
const exceededLimit = counter >= getLimitFromEvent(event);
const executions = [];
for (const executionArn of runningExecutionArns) {
Copy link
Contributor

@marchuffnagle marchuffnagle Mar 14, 2018

Choose a reason for hiding this comment

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

The style guide recommends avoiding iterators when possible. Instead of a for loop here, consider using map.

http://airbnb.io/javascript/#iterators--nope

Because of the special case where the execution does not yet exist, I would probably create a describeExecutionStatus function and use that in the map. Something like:

function describeExecutionStatus(executionArn) {
  return aws.sfn().describeExecution({ executionArn }).promise()
    .catch((e) => {
      if (e.code === 'ExecutionDoesNotExist') {
        return { executionArn: executionArn, status: 'RUNNING' };
      }
      throw e;
    });
}

With that function, you could then remove the loop and just say

const executions = await Promise.all(promisedExecutionStatuses);

The other change this will make is that all of the describeExecution operations will be done in parallel, not one at a time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! I realized we should keep Promise.all after I started to look at CUMULUS-304. I will do this under CUMULUS-304: Add AWS API throttling to pdr-status-check task.

Copy link
Contributor

Choose a reason for hiding this comment

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

if we don’t use a for loop here we should consider using p-limit or we will quickly hit the step function api limit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@scisco I will fix it under CUMULUS-304.

try {
const execution = await aws.sfn().describeExecution({ executionArn }).promise();
executions.push(execution);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should } catch (e) { should be on one line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

eslint doesn't like the one-line style

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

That's because of this override: https://github.com/cumulus-nasa/cumulus/blob/master/.eslintrc.json#L86.

Our eslintrc is requiring the "stroustrup" style for braces: https://eslint.org/docs/rules/brace-style

catch (e) {
// it's ok if a execution is still in the queue and has not be executed
if (e.code === 'ExecutionDoesNotExist') {
executions.push({ executionArn: executionArn, status: 'RUNNING' });
}
Copy link
Contributor

@abarciauskas-bgse abarciauskas-bgse Mar 13, 2018

Choose a reason for hiding this comment

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

Same thing here

} else {
  throw e;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, our eslint doesn't like that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as the other instance:

That's because of this override: https://github.com/cumulus-nasa/cumulus/blob/master/.eslintrc.json#L86.

Our eslintrc is requiring the "stroustrup" style for braces: https://eslint.org/docs/rules/brace-style

else throw e;
}
}

const executionsAllDone = groupedExecutions.running.length === 0;
const groupedExecutions = groupExecutionsByStatus(executions);
const counter = getCounterFromEvent(event) + 1;
const exceededLimit = counter >= getLimitFromEvent(event);

if (!executionsAllDone && exceededLimit) {
throw new IncompleteError(`PDR didn't complete after ${counter} checks`);
}
const executionsAllDone = groupedExecutions.running.length === 0;
if (!executionsAllDone && exceededLimit) {
throw new IncompleteError(`PDR didn't complete after ${counter} checks`);
}

const output = buildOutput(event, groupedExecutions);
if (!output.isFinished) logStatus(output);
return output;
});
const output = buildOutput(event, groupedExecutions);
if (!output.isFinished) logStatus(output);
return output;
}
exports.checkPdrStatuses = checkPdrStatuses;

Expand Down
1 change: 1 addition & 0 deletions cumulus/tasks/pdr-status-check/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
]
},
"dependencies": {
"@cumulus/api": "^1.1.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not used any where. Please remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

"@cumulus/common": "^1.1.0",
"@cumulus/cumulus-message-adapter-js": "^1.0.1",
"@cumulus/ingest": "^1.1.1",
Expand Down
10 changes: 9 additions & 1 deletion cumulus/tasks/pdr-status-check/schemas/input.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,14 @@
},
"counter": { "type": "integer" },
"limit": { "type": "integer" },
"isFinished": { "type": "boolean" }
"isFinished": { "type": "boolean" },
"pdr": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added description and made it a required field.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please make this property required.

"type": "object",
"required": ["name", "path"],
"properties": {
"name": { "type": "string" },
"path": { "type": "string" }
}
}
}
}
10 changes: 9 additions & 1 deletion cumulus/tasks/pdr-status-check/schemas/output.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,14 @@
},
"counter": { "type": "integer" },
"limit": { "type": "integer" },
"isFinished": { "type": "boolean" }
"isFinished": { "type": "boolean" },
"pdr": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment and make this a required property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

"type": "object",
"required": ["name", "path"],
"properties": {
"name": { "type": "string" },
"path": { "type": "string" }
}
}
}
}
34 changes: 23 additions & 11 deletions cumulus/tasks/pdr-status-check/tests/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ const { checkPdrStatuses } = require('../index');
test('valid output when no running executions', (t) => {
const event = {
input: {
running: []
running: [],
pdr: { name: 'test.PDR', path: 'test-path' }
}
};

Expand All @@ -19,7 +20,8 @@ test('valid output when no running executions', (t) => {
isFinished: true,
running: [],
failed: [],
completed: []
completed: [],
pdr: { name: 'test.PDR', path: 'test-path' }
};

t.deepEqual(output, expectedOutput);
Expand All @@ -41,7 +43,8 @@ test('error thrown when limit exceeded', (t) => {
input: {
running: ['arn:123'],
counter: 2,
limit: 3
limit: 3,
pdr: { name: 'test.PDR', path: 'test-path' }
}
};

Expand All @@ -61,26 +64,35 @@ test('returns the correct results in the nominal case', (t) => {
'arn:1': 'RUNNING',
'arn:2': 'SUCCEEDED',
'arn:3': 'FAILED',
'arn:4': 'ABORTED'
'arn:4': 'ABORTED',
'arn:7': null
};
const error = {
message: 'Execution Does Not Exist: arn',
Copy link
Contributor

Choose a reason for hiding this comment

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

This is always going to set the message to Execution Does Not Exist: arn, not the actual execution arn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

code: 'ExecutionDoesNotExist'
};

const stubSfnClient = {
describeExecution: ({ executionArn }) => ({
promise: () => Promise.resolve({
executionArn,
status: executionStatuses[executionArn]
})
promise: () => {
if (executionStatuses[executionArn] === null) return Promise.reject(error);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be returning an actual Error instance, not a simple object. Consider something like:

if (!executionStatuses[executionArn]) {
  const error = new Error(`Execution does not exist: ${executionArn}`);
  error.code = 'ExecutionDoesNotExist';
  Promise.reject(error);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Done

return Promise.resolve({
executionArn,
status: executionStatuses[executionArn]
});
}
})
};
const stub = sinon.stub(aws, 'sfn').returns(stubSfnClient);

const event = {
input: {
running: ['arn:1', 'arn:2', 'arn:3', 'arn:4'],
running: ['arn:1', 'arn:2', 'arn:3', 'arn:4', 'arn:7'],
completed: ['arn:5'],
failed: [{ arn: 'arn:6', reason: 'OutOfCheese' }],
counter: 5,
limit: 10
limit: 10,
pdr: { name: 'test.PDR', path: 'test-path' }
}
};

Expand All @@ -92,7 +104,7 @@ test('returns the correct results in the nominal case', (t) => {
t.is(output.counter, 6);
t.is(output.limit, 10);

t.deepEqual(output.running, ['arn:1']);
t.deepEqual(output.running, ['arn:1', 'arn:7']);
t.deepEqual(output.completed.sort(), ['arn:2', 'arn:5'].sort());

t.is(output.failed.length, 3);
Expand Down
55 changes: 55 additions & 0 deletions cumulus/tasks/sf-sns-report/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
# @cumulus/queue-pdrs
Copy link
Contributor

Choose a reason for hiding this comment

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

The name is incorrect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

corrected


[![CircleCI](https://circleci.com/gh/cumulus-nasa/cumulus.svg?style=svg)](https://circleci.com/gh/cumulus-nasa/cumulus)

Broadcast an incoming Cumulus message to SNS. This lambda function works with Cumulus Message Adapter, and it can be used anywhere in a step function workflow to report granule and PDR status.

To report the PDR's progress as it's being processed, add the following step after the pdr-status-check:

PdrStatusReport:
CumulusConfig:
cumulus_message:
input: '{$}'
outputs:
- source: '{$.payload}'
destination: '{$.payload}'
Type: Task
Resource: ${SfSnsReportLambdaFunction.Arn}

To report the start status of the step function:

StartAt: StatusReport
States:
StatusReport:
CumulusConfig:
cumulus_message:
input: '{$}'
outputs:
- source: '{$.payload}'
destination: '{$.payload}'
Type: Task
Resource: ${SfSnsReportLambdaFunction.Arn}

To report the final status of the step function:

StopStatus:
CumulusConfig:
sfnEnd: true
stack: '{$.meta.stack}'
bucket: '{$.meta.buckets.internal}'
stateMachine: '{$.cumulus_meta.state_machine}'
executionTime: '{$.cumulus_meta.execution_name}'
cumulus_message:
input: '{$}'
Type: Task
Resource: ${SfSnsReportLambdaFunction.Arn}

## What is Cumulus?

Cumulus is a cloud-based data ingest, archive, distribution and management prototype for NASA's future Earth science data streams.

[Cumulus Documentation](https://cumulus-nasa.github.io/)

## Contributing

See [Cumulus README](https://github.com/cumulus-nasa/cumulus/blob/master/README.md#installing-and-deploying)
127 changes: 127 additions & 0 deletions cumulus/tasks/sf-sns-report/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
'use strict';

const get = require('lodash.get');
const { StepFunction } = require('@cumulus/ingest/aws');
const { setGranuleStatus, sns } = require('@cumulus/common/aws');
const errors = require('@cumulus/common/errors');
const cumulusMessageAdapter = require('@cumulus/cumulus-message-adapter-js');

/**
* Determines if there was a valid exception in the input message
*
* @param {Object} event - aws event object
* @returns {boolean} true if there was an exception, false otherwise
*/
function eventFailed(event) {
if (event.exception) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe flatten these nested conditionals?

if (event.exception && typeof(event.exception) === 'object' && Object.keys(event.exception.length) > 0) {
  return true
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. this is neat

if (typeof event.exception === 'object') {
// this is needed to avoid flagging cases like "exception: {}" or "exception: 'none'"
if (Object.keys(event.exception).length > 0) {
return true;
}
}
}
// Error and error keys are not part of the cumulus message
// and if they appear in the message something is seriously wrong
else if (event.Error || event.error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know an instance of this occurring? A better option might be to whitelist the top-level keys we expect from a cumulus message, otherwise this seems unnecessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't. I don't know if removing this logic will break something.

Copy link
Contributor

Choose a reason for hiding this comment

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

this was reported in an earlier bug report and this is the fix for that bug.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah ok - do you recall if that bug was before or after cumulus message adapter integration?

Copy link
Contributor

@marchuffnagle marchuffnagle Mar 15, 2018

Choose a reason for hiding this comment

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

@abarciauskas-bgse brings up a very good point. When you're using the message adapter, the only keys that the simplified event is going to have are "config" and "input". This is never going to trigger.

return true;
}
return false;
}

/**
* if the cumulus message shows that a previous step failed,
* this function extract the error message from the cumulus message
Copy link
Contributor

Choose a reason for hiding this comment

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

this function extracts the error message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

* and fail the function with that information. This ensures that the
Copy link
Contributor

Choose a reason for hiding this comment

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

and fails the function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

* Step Function workflow fails with the correct error info
*
* @param {Object} event - aws event object
* @returns {undefined} throws an error and does not return anything
*/
function makeLambdaFunctionFail(event) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a little hard to follow what this function is doing. It might be easier with something like:

function buildError(type, cause) {
  let ErrorClass;

  if (Object.keys(errors).includes(type)) ErrorClass = errors[type];
  else if (type === 'TypeError') ErrorClass = TypeError;
  else ErrorClass = Error;

  return new ErrorClass(cause);
}

function makeLambdaFunctionFail(event) {
  const error = event.exception || event.error;

  if (!error) throw new Error('Step Function failed for an unknown reason.');
  
  throw buildError(error.Error, error.Cause);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. This makes the code much clear. Sorry I didn't try to understand the code.

const error = get(event, 'exception.Error', get(event, 'error.Error'));
const cause = get(event, 'exception.Cause', get(event, 'error.Cause'));
if (error) {
if (errors[error]) {
throw new errors[error](cause);
}
else if (error === 'TypeError') {
throw new TypeError(cause);
}
throw new Error(cause);
}

throw new Error('Step Function failed for an unknown reason.');
}

/**
* Publishes incoming Cumulus Message in its entirety to
* a given SNS topic
*
* @param {Object} event - a Cumulus Message that has been sent through the
* Cumulus Message Adapter. See schemas/input.json for detailed input schema.
* @param {Object} event.config - configuration object for the task
* @param {Object} event.config.sfnEnd - indicate if it's the last step of the step function
* @param {string} event.config.stack - the name of the deployment stack
* @param {string} event.config.bucket - S3 bucket
* @param {string} event.config.stateMachine - current state machine
* @param {string} event.config.executionTime - execution time
* @returns {Promise.<Object>} - AWS SNS response. see schemas/output.json for detailed output
* schema that is passed to the next task in the workflow
*/
async function publishSnsMessage(event) {
const config = get(event, 'config', []);
Copy link
Contributor

Choose a reason for hiding this comment

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

The config is an Object, not an Array. That being said, message adapter will ensure that config is always set, so there's no need for a default value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

const message = get(event, 'input', []);
Copy link
Contributor

Choose a reason for hiding this comment

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

The input is an Object, not an Array. That being said, message adapter will ensure that config is always set, so there's no need for a default value.


const finished = get(config, 'sfnEnd', false);
const topicArn = get(message, 'meta.topic_arn', null);
const failed = eventFailed(message);

if (topicArn) {
// if this is the sns call at the end of the execution
if (finished) {
message.meta.status = failed ? 'failed' : 'completed';
const granuleId = get(message, 'meta.granuleId', null);
if (granuleId) {
await setGranuleStatus(
granuleId,
get(config, 'stack', null),
Copy link
Contributor

Choose a reason for hiding this comment

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

If any of these values are not set, you're passing null in their place. What is setGranuleStatus going to do with a null bucket, or any of the other parameters for that matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the code and schema, there is no good way to prevent the user from misconfiguring the parameters.

get(config, 'bucket', null),
get(config, 'stateMachine', null),
get(config, 'executionName', null),
message.meta.status
);
}
}
else {
message.meta.status = 'running';
}

await sns().publish({
TopicArn: topicArn,
Message: JSON.stringify(message)
}).promise();
}

if (failed) {
makeLambdaFunctionFail(message);
}

return message;
Copy link
Contributor

Choose a reason for hiding this comment

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

@jennyhliu I think we should NOT return the whole message here. If the user forgets to include the outputs key in the cumulus config, the message ends up getting larger and larger.

Could you instead just return the output of the sns aws call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the code to return the output from sns response, and the README.md

}

exports.publishSnsMessage = publishSnsMessage;

/**
* Lambda handler. It broadcasts an incoming Cumulus message to SNS
*
* @param {Object} event - a Cumulus Message
* @param {Object} context - an AWS Lambda context object
* @param {Function} callback - an AWS Lambda call back
* @returns {Promise} updated event object
*/
function handler(event, context, callback) {
return StepFunction.pullEvent(event).then((message) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: wrap in curly brace:

return StepFunction.pullEvent(event).then((message) => {
  cumulusMessageAdapter.runCumulusTask(publishSnsMessage, message, context, callback));
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

cumulusMessageAdapter.runCumulusTask(publishSnsMessage, message, context, callback));
}
exports.handler = handler;
Loading