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
[STRMCMP-1639] Integration Test Fix #277
[STRMCMP-1639] Integration Test Fix #277
Changes from 58 commits
a0a90a0
0cc931c
3859dc3
33234ca
a43addf
698bc34
4e81a8d
cb21077
8a7811b
68b2330
b0eede8
490f385
f821e91
3e6dbfe
5101df2
e904616
0196be5
abc7186
3965f45
74bebca
bca42b4
02ebaef
c469f10
bd5a7b7
b3fabc6
f425da7
5350c37
3110b64
f6cc040
15787c6
521bac5
2c09fb2
4cb11e5
80b8251
38d31d6
9ea26a8
9a2f36b
6a8b657
f4bfd5b
f1d51ff
532906d
b5d6626
eb7ede2
238f0ce
cd0b4b1
5fc7c04
09b094d
54c61af
13ac8c0
138b0a4
73a17cf
b1dfd96
c928440
de9651f
1dd4788
63b6797
1cbc3f6
3db9f0c
6e86f70
7fab7cc
11fbbe8
69ccd27
21b7f7b
2579200
48b2ecf
aba457f
db88cf9
1db3074
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.
Maybe just add a TODO here with the jira to upgrade client?
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.
Created a ticket for the 1.22 upgrade and added
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 thought
IntegTest
is not needed.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 removed this when attempting to target a single test. No luck there for now so leaving as is since this is what was there for the integration test prior. In the future we'll figure out how to target a single test as I'll likely write an integration test for fallback without savepoint
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 don't get this test, you're creating and modifying privs on a file and then just asserting the commands ran without errors?
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.
Changes the behavior of the flink app: https://github.com/lyft/flinkk8soperator/blob/master/integ/operator-test-app/src/main/java/com/lyft/OperatorTestApp.java#L92
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.
Don't have to change anything but generally speaking I am never a fan of such sleep timers in tests (just makes it flakey).
I would always promote to change this to an polling method with exponential delay (and an exit criteria). Again, don't need to do that now but just wanted to point it out.
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.
Agreed. Will leave as a future update or at least keep in mind while writing future integration tests for this repo