Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: Support AWS usage marketplace #1770
feat: Support AWS usage marketplace #1770
Changes from 13 commits
44c1501
a344de8
38ba207
284759f
effb581
f23c30c
f18f71f
5732cf9
e1bfb03
0c552b7
102aa07
310240c
4aa4422
9ea3d5b
663fccd
566bbc0
9076bea
d5ab937
55382b8
a8a453e
1cd7d2a
2727ba1
b0e32fc
31f1845
079ed11
87051fd
c8be2ae
15281cc
5730106
45d9384
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
What's the reason for using
context.TODO()
in favor ofcontext.Background()
? Maybe we should add timeout as well for loading the config?Also, can we put this logic in a
setupAWSMarketplace
function asNewUsageClient
is getting quite longThere 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.
context.TODO()
is supposed to be used when the surrounding function doesn't have a context available to use, so that is why I used it here. I am not sure the value of a timeout would add here.I have moved the logic to setupAWSMarketplace in 566bbc0
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.
I'm not sure this is necessary, but would need to verify. I think how it should work is that there is a minimum flush time that should come into effect (by default this is 10s)
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.
I think there is a race condition, if a batch is flushed and within the same second the sync ends then the timestamp will have the same value as timestamps are truncated to the second
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.
Ah, good catch... ok this should work for now but we should probably try and think of a better solution in the long term, otherwise we won't count any records if the sync is abruptly stopped