Skip to content

Conversation

@ab295382
Copy link
Member

@ab295382 ab295382 commented Jul 24, 2025

Issue

Tests

  • My PR adds the following tests OR does not need testing for this extremely good reason:
    • MyUnitTest

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it, or I have linked to a
    separate issue for that below.
  • If I have added or removed any dependencies from the project, I have updated the NOTICES file.

@ab295382 ab295382 linked an issue Jul 24, 2025 that may be closed by this pull request
@ab295382 ab295382 changed the title 4472 - Adding first attempt to reduce duplication code for compaction… 4472 - Reduce duplication code for compactions and bulk export Jul 28, 2025
@patchwork01 patchwork01 added the needs-reviewer Pull requests that need a reviewer to be assigned label Jul 31, 2025
@patchwork01 patchwork01 removed the needs-reviewer Pull requests that need a reviewer to be assigned label Jul 31, 2025
@ab295382 ab295382 marked this pull request as ready for review August 6, 2025 10:52
@ab295382 ab295382 force-pushed the 4472-reduce-duplication-between-starting-bulk-export-and-compaction-tasks branch from bb5b2f0 to 06d4e4e Compare August 6, 2025 15:14
@ca61688 ca61688 marked this pull request as ready for review September 8, 2025 16:21
@ca61688 ca61688 assigned patchwork01 and unassigned ab295382 Sep 8, 2025
@patchwork01 patchwork01 assigned ca61688 and unassigned patchwork01 Sep 9, 2025
@ca61688 ca61688 marked this pull request as draft September 10, 2025 07:28
@ca61688 ca61688 force-pushed the 4472-reduce-duplication-between-starting-bulk-export-and-compaction-tasks branch from 8c3ec27 to e919608 Compare September 10, 2025 13:45
@ca61688 ca61688 marked this pull request as ready for review September 10, 2025 15:37
@ca61688 ca61688 assigned patchwork01 and unassigned ca61688 Sep 10, 2025
Copy link
Collaborator

@patchwork01 patchwork01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the only important thing left is the argument count checks in the main methods. Other than that this looks great. Have you tested it in AWS, for bulk export & compaction?

@patchwork01 patchwork01 assigned ca61688 and unassigned patchwork01 Sep 11, 2025
@ca61688
Copy link
Collaborator

ca61688 commented Sep 16, 2025

I think the only important thing left is the argument count checks in the main methods. Other than that this looks great. Have you tested it in AWS, for bulk export & compaction?

Haven't tested in AWS but happy to give that a go, might need some direction on doing that though

@ca61688 ca61688 assigned patchwork01 and unassigned ca61688 Sep 16, 2025
Copy link
Collaborator

@patchwork01 patchwork01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks. I've left a couple more minor comments. It would be good to test it in AWS before we merge it.

For compaction there are some acceptance system tests that cover this:

  • CompactionST
  • CompactionOnEC2ST

We don't currently have a system test for bulk export, so you'd need to use the deployAll manual testing setup for that. You'll need to get some data into a table (which the deployAll setup does for you by default), and then trigger a bulk export by sending a message to the queue. Here's the documentation for that:

https://github.com/gchq/sleeper/blob/develop/docs/usage/export.md

Here's the documentation for those two types of system testing (acceptance tests vs manual testing):

https://github.com/gchq/sleeper/blob/develop/docs/development/system-tests.md

return;
}
String instanceId = args[0];
int numberOfTasks = Integer.parseInt(args[1]);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a bit easier to read with the arguments brought out like this, since you can compare it against the usage and count check just above here more directly. Can we keep this as it was before? The same applies to BulkExportTaskHostScaler.

@patchwork01 patchwork01 assigned ca61688 and unassigned patchwork01 Sep 18, 2025
@ca61688
Copy link
Collaborator

ca61688 commented Sep 24, 2025

Looks good, thanks. I've left a couple more minor comments. It would be good to test it in AWS before we merge it.

For compaction there are some acceptance system tests that cover this:

  • CompactionST
  • CompactionOnEC2ST

We don't currently have a system test for bulk export, so you'd need to use the deployAll manual testing setup for that. You'll need to get some data into a table (which the deployAll setup does for you by default), and then trigger a bulk export by sending a message to the queue. Here's the documentation for that:

https://github.com/gchq/sleeper/blob/develop/docs/usage/export.md

Here's the documentation for those two types of system testing (acceptance tests vs manual testing):

https://github.com/gchq/sleeper/blob/develop/docs/development/system-tests.md

Okay I've now tested it on aws and the system tests ran fine along with a bulk export manual check. I think this is ready to merge now

@patchwork01 patchwork01 merged commit 82f9c8a into develop Sep 25, 2025
6 checks passed
@patchwork01 patchwork01 deleted the 4472-reduce-duplication-between-starting-bulk-export-and-compaction-tasks branch September 25, 2025 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reduce duplication between starting bulk export and compaction tasks

4 participants