Skip to content
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

Support batch_size per class #9

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

chrispenny
Copy link
Contributor

@chrispenny chrispenny commented Nov 14, 2024

Batch size by class and index

Different classes can require different levels of server resource to process, for example, Files are often more process intensive than (say) Pages.

Provide developers with a way to define a batch_size to be used when re-indexing batches of certain classes.

Unit test changes

I'm sure they were fine before, but they used features I don't really understand - so, some have been updated to use test mechanisms that I understand.

@chrispenny chrispenny force-pushed the feature/class-batch-size-config branch 2 times, most recently from fd772e1 to 6c23a4a Compare November 14, 2024 01:11
@chrispenny chrispenny force-pushed the feature/class-batch-size-config branch 9 times, most recently from fcbbf81 to 3144c2e Compare November 18, 2024 02:27
@chrispenny chrispenny marked this pull request as ready for review November 18, 2024 02:31
@chrispenny chrispenny force-pushed the feature/class-batch-size-config branch 3 times, most recently from 62f6643 to c5f6ee3 Compare November 18, 2024 18:51
Copy link
Contributor

@blueo blueo left a comment

Choose a reason for hiding this comment

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

ah man turns out I forgot to hit the review button.... anyway - the batch changes look all good but I'm wondering about the cooldown. I'm a bit worried this will block a queue for potentially a long time and we could leverage queued jobs for this instead?

src/Jobs/BatchJob.php Show resolved Hide resolved
@chrispenny chrispenny force-pushed the feature/class-batch-size-config branch 5 times, most recently from f624dd0 to 5fbe31a Compare December 1, 2024 18:54
@chrispenny chrispenny force-pushed the feature/class-batch-size-config branch from 5fbe31a to 062eaaa Compare December 15, 2024 18:30
@chrispenny chrispenny changed the base branch from main to feature/batch-cooldown December 15, 2024 19:07
@chrispenny chrispenny force-pushed the feature/class-batch-size-config branch from 062eaaa to 97cfe2c Compare December 15, 2024 19:07
@chrispenny chrispenny changed the title Support batch_size per class. Support a cooldown between batches Support batch_size per class Dec 15, 2024
@chrispenny chrispenny force-pushed the feature/class-batch-size-config branch from 97cfe2c to 987de78 Compare December 15, 2024 19:10
@chrispenny chrispenny marked this pull request as draft December 15, 2024 19:14
@chrispenny
Copy link
Contributor Author

chrispenny commented Dec 15, 2024

Converting back to draft because I've had a thought that I'd like to look further in to.

  • I think it might be possible to have the batch_size by class added to the DataObjectFetcher instead of it living at the Job level
  • If possible, this would mean that each step in the job could use a different batch size (based on the fetcher being used for that particular step)

Edit, after some investigation:

  • I think in order to "do it right" with Fetchers, we would need to add "batch size" as part of the DocumentFetcherInterface
  • Adding any requirements to an interface would be a breaking change, so I don't think we can do this without a new MAJOR
  • CMS 6 is coming up early next year, and we might choose to add support for that through a new MAJOR
  • It migth make sense to bundle this "fetcher batch size" feature in with the MAJOR for CMS 6

I think the feature (as we have it right now in this PR) is a good middleground that doesn't cause any breaking changes. I like the way that this new feature configures batch sizes per class, and I think that would persist (even if we move where the batching occurs)

Raised a follow up Issue:
#12

@chrispenny chrispenny force-pushed the feature/class-batch-size-config branch 2 times, most recently from 5693c1f to f35c662 Compare December 15, 2024 19:20
@chrispenny chrispenny force-pushed the feature/batch-cooldown branch from fc64778 to 4a6b23c Compare December 15, 2024 19:22
@chrispenny chrispenny force-pushed the feature/class-batch-size-config branch 2 times, most recently from 5261e5d to 7dc5d9f Compare December 15, 2024 19:29
Base automatically changed from feature/batch-cooldown to main December 15, 2024 19:31
@chrispenny chrispenny force-pushed the feature/class-batch-size-config branch from 7dc5d9f to 177030b Compare December 16, 2024 00:26
@chrispenny chrispenny marked this pull request as ready for review January 13, 2025 17:43
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.

2 participants