-
Notifications
You must be signed in to change notification settings - Fork 2.5k
chore(ci): Hudi-utilities test improvements #17758
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
chore(ci): Hudi-utilities test improvements #17758
Conversation
| command: 'run' | ||
| arguments: > | ||
| -v $(Build.SourcesDirectory):/hudi | ||
| -v /var/run/docker.sock:/var/run/docker.sock |
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 is required for running docker in docker
|
|
||
| import scala.Tuple2; | ||
|
|
||
| public class KafkaTestUtils { |
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.
Drop in replacement for the existing KafkaTestUtils but does not require zookeeper
| HoodieTableMetaClient meta = createMetaClient(storage, tablePath); | ||
| HoodieTimeline timeline = meta.getActiveTimeline().getCommitAndReplaceTimeline().filterCompletedInstants(); | ||
| LOG.info("Timeline Instants=" + meta.getActiveTimeline().getInstants()); | ||
| LOG.info("Timeline Instants={}", meta.getActiveTimeline().getInstants()); |
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.
Fixed logging to avoid the toString calls while I was updating this file
| <scope>compile</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.testcontainers</groupId> |
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.
Moved this to be where it is required
| HoodieStreamer streamer = new HoodieStreamer(context.getConfig(), jssc, Option.ofNullable(context.getProperties())); | ||
| streamer.sync(); | ||
| successTables.add(Helpers.getTableWithDatabase(context)); | ||
| streamer.shutdownGracefully(); |
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.
Ensure the instances are shutdown properly
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.
minor. should we move this to finally block?
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, makes sense
| // Initial bulk insert to ingest to first hudi table | ||
| HoodieDeltaStreamer.Config cfg = getConfig(tableBasePath, getSyncNames("MockSyncTool1", "MockSyncTool2")); | ||
| new HoodieDeltaStreamer(cfg, jsc, fs, hiveServer.getHiveConf()).sync(); | ||
| syncOnce(new HoodieDeltaStreamer(cfg, jsc, fs, hiveServer.getHiveConf())); |
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.
Lots of changes in this PR are using utils or other means to ensure the streamer is shutdown
|
|
||
| Map<String, String> hudiOpts = new HashMap<>(); | ||
| public KafkaTestUtils testUtils; | ||
| protected static KafkaTestUtils testUtils; |
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.
Tests are now only setting up kafka once to cut down on the overhead
nsivabalan
left a comment
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.
nice job on the fixes and test run time reduction
| HoodieStreamer streamer = new HoodieStreamer(context.getConfig(), jssc, Option.ofNullable(context.getProperties())); | ||
| streamer.sync(); | ||
| successTables.add(Helpers.getTableWithDatabase(context)); | ||
| streamer.shutdownGracefully(); |
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.
minor. should we move this to finally block?
|
|
||
| @AfterEach | ||
| public void cleanupClass() { | ||
| @AfterAll |
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 keep AfterEach and add testUtils.deleteTopics(); in that?
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 generally doesn't make any impact since the topics are so small but I can add it
|
|
||
| @AfterEach | ||
| public void tearDown() { | ||
| @AfterAll |
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 keep AfterEach and add testUtils.deleteTopics(); in that?
| testUtils.setup(); | ||
| } | ||
|
|
||
| @AfterEach |
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 keep AfterEach and add testUtils.deleteTopics(); in that?
|
|
||
| @AfterEach | ||
| public void teardown() throws Exception { | ||
| @AfterAll |
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.
same comment as above.
c02717e to
6dfb53d
Compare
Describe the issue this Pull Request addresses
We see occasional timeouts with the utilities tests in Azure CI so this PR aims to improve the test runtimes to ensure CI is not flakey.
Summary and Changelog
TestHoodieDeltaStreamerfrom 31 minutes to 12 minutes on my local machine.Impact
Improve CI stability
Risk Level
None
Documentation Update
Contributor's checklist