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
🐛 Source salesforce: failed async bulk jobs #8009
🐛 Source salesforce: failed async bulk jobs #8009
Changes from all commits
0ce5182
3d5c352
f2f748c
8a42371
3aa98c1
b93c8fa
b1b42d3
5f8bba3
4dcd0e4
350a7fc
9fe749a
4a17ae8
ded7204
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.
should we also do this in templates? (can be separate pr)
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 planning to aggregate more improvements for a separate PR)) for example I want to fix an issue with running of custom acceptance tests locally. These tests can be executed by GitHub CI only and developers can't run them by the script
acceptance-test-docker.sh
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 did the default value change? is it meaningful to call this without a stream name supplied?
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.
IMO this option should not be exposed - it is an implementation detail. See the UX handbook for more context behind why it's not a good idea to expose it. Maybe instead we can use a dynamically increasing wait time for each job.
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.
Thus I forwarded this PR to Airbyte team)
Your idea with autoscaling of waiting timeout is pretty but we can reproduce this case with long responses locally. For more details I added more informative log messages for troubleshooting of possible similar cases.
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.
every time we pass a time duration we should either use a data type which encodes the unit of time or we should embed the unit of time in the var name e.g:
wait_timeout_in_minutes
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.
can you use a more expressive variable name?
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.
signature should be
-> Optional[str]
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.
it's probably better to encode these job statuses using an enum
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.
Yes, it was the bug-fixing only with minimal refactoring. Sure we can do it while a next review.
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.
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.
shouldn't we be retrying this job instead of failing?
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 was not sure about possible reasons of failures. Unfortunately I didn't succeed to catch any real failed or aborted job. Maybe there are some internal job failure(timeout or resource issues)