-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Conversation
@@ -130,10 +133,13 @@ TEST_TEAR_DOWN( Full_DEFENDER ) | |||
{ | |||
AwsIotDefender_Stop(); | |||
|
|||
if( _reportAccepted || _reportRejected ) | |||
/* Actually get defender callback. */ | |||
if( _callbackInfo.eventType != -1 ) |
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.
Chatted offline. It seems in this implementation, we leave throttling handling to user. To be specific, application is increasing _DEFENDER_PUBLISH_INTERVAL_SECONDS, and parsing _callbackInfo.eventType to determine what to do next.
This may not be the best solution. Since:
- This creates unreliable test. Throttling is usually per region per account, a fix number with single attempt would never reliably work.
- Inconsistent with other AWS clients. Where usually in other clients, users can input retry scheme, and max retry. (e.g. typical retry scheme -- exponential backoff delay. typical max retry -- 3.)
This review comment is more about design instead of this specific PR. Apologies.
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.
_reportRejected = pCallbackInfo->eventType == AWS_IOT_DEFENDER_METRICS_REJECTED; | ||
if( _callbackInfo.payloadLength > 0 ) | ||
{ | ||
_callbackInfo.pPayload = AwsIotTest_Malloc( _callbackInfo.payloadLength ); |
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.
line 503, 512, 533, and 534. Symmetry is beauty. Better to put allocate/free both in test setup/teardown. Or both in local test.
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.
This makes sense. I can move the allocate to setup
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.
Generic comments provided. Approving as a non content expert.
Description
Checklist: