Spark 3.2: Refactor TestRemoveOrphanFilesAction - remove Thread.sleep()#4711
Conversation
|
Looks reasonable to me, if its just for tests, should we just do all the spark versions in one pr to reduce overhead? Not sure what people think. |
|
I'm ok with either all versions or back-porting by version. Technically by version is the standard but for small things we do often do multiple versions at once. Side question: Am I correct in assuming that for cherry-picking in a fork that all versions at once for smaller things is preferred for your workflow @szehon-ho? Ideally we do whatever is most practical for end users (within reason), which includes people who actively maintain forks. |
|
|
||
| // sleep for 1 second to unsure files will be old enough | ||
| Thread.sleep(1000); | ||
| waitUntilAfter(System.currentTimeMillis()); |
There was a problem hiding this comment.
I think this will jsut immediately return. The waitUntilAfter function loops while the passed in timestamp is less than System.currentTimeMillis.
Other places I see waitUntilAfter used tend to wait until table.currentSnapshot.timestampMillis(). But in this file it does seem that this is how waitUntilAfter is used, so I'm ok with this.
If we notice this being flakey, then we might want to change it to using the table's current snapshot timestamp instead.
|
@kbendick no strong opinion or workflow (my only thought there was ease of reviewing), I'm fine to do separate prs as well. |
|
Thanks, @ulmako! Good to have this fixed. |
4 participants
Follow up to PR #4307 refactoring the TestRemoceOrphanFilesAction class and replace
thread.sleep()with calls towaitUntilAfter()method.