Configurable batch size for `JIRA._fetch_pages()` and dependant methods #1394

rynkk · 2022-06-03T14:38:55Z

Add batch_size argument to JIRA._fetch_pages and dependant methods.
This allows for significant performance improvements when trying to retrieve a lot of items using maxResults=False.

fixes #1393

The program was tested solely for our own use cases, which might differ from yours.

_{Jannik Meinecke (jannik.meinecke@mercedes-benz.com) on behalf of MBition GmbH.

Provider Information}

Licensed under BSD-2-Clause license

___ Jannik Meinecke <jannik.meinecke@mercedes-benz.com> on behalf of MBition GmbH. https://github.com/mercedes-benz/foss/blob/master/PROVIDER_INFORMATION.md

rynkk · 2022-06-03T14:46:52Z

Required change in _fetch_pages was a lot less than anticipated. I improved the method's tests, while I was at it.

_{Jannik Meinecke (jannik.meinecke@mercedes-benz.com) on behalf of MBition GmbH.

Provider Information}

jira/client.py

studioj · 2022-06-04T18:19:23Z

I'm not sure what this entails ... it seems just like offering a different way of doing the same thing 🤔
Could you update the examples maybe to explain the usecase a little bit?
I dont really like using maxResults and batchsize to set the maxResults in the json, but that's personal of course ...

studioj · 2022-06-05T10:36:31Z

Maybe setting a batch size for the whole jira class seems more logical instead of adding it for quite some methods? =>
Would there be a disadvantage for having a large batch size on shorter queries?
I see your improvement from 41s to 6s is pretty significant so its definitly worth continuing this train of thought.
I'm just trying to prevent tons of parameters being added to methods... It's often seen as a code smell if your parameter list is getting too large ...

If you would be in dire need for a speedup quickly, probably a quick fix would be to increase the default batch size to 100 🤔?
although I like it to be configurable

@adehad @ssbarnea some architectural guidance is welcome :-)

adehad

Maybe setting a batch size for the whole jira class seems more logical instead of adding it for quite some methods? => Would there be a disadvantage for having a large batch size on shorter queries? I see your improvement from 41s to 6s is pretty significant so its definitly worth continuing this train of thought. I'm just trying to prevent tons of parameters being added to methods... It's often seen as a code smell if your parameter list is getting too large ...

If you would be in dire need for a speedup quickly, probably a quick fix would be to increase the default batch size to 100 🤔? although I like it to be configurable

@adehad @ssbarnea some architectural guidance is welcome :-)

@rynkk, thanks for this. Especially as this is one of the more convoluted areas in the code.
As suggested by @studioj I believe this batch size needs to be set at a class instance level. i.e. in here something like DEFAULT_BATCH_SIZE

jira/jira/client.py

Line 343 in 6789038

DEFAULT_OPTIONS = {

I completely understand where you are coming from with wanting to set this per function. It provides the most flexibility, especially as you will likely have more issues than dashboards and therefore would want to set it differently for different resources. Also scales differently depending on how fast the server is you are working with.

A solution I would prefer if you strongly would like to set this per endpoint might look like so:

# within the DEFAULT_OPTIONS

# A dict of (Resource Type, Batch Size) 
DEFAULT_BATCH_SIZE: Dict[Resource, int] = {
    Issue : 500,
    Resource: 100, # or 50 or whatever we want as a default now
}


...

# inside the JIRA.__init__

self.options["default_batch_size"] = DEFAULT_BATCH_SIZE
self.options["default_batch_size"].update(default_batch_sizes)  # or whatever the argument name is
 



...
# now inside fetch pages

        if maxResults:
            page_params["maxResults"] = maxResults
        else: # will need to set batch size to get all resources
            page_params["maxResults"] = self.options["default_batch_size"].get(item_type, self.options["default_batch_size"].get(Resource))
            _LOG.verbose(f"Batch size for {item_type.__name__} set to {page_params["maxResults"]}")

There might be problems with this approach too, but I feel it may be more ergonomic for advanced users such as yourself without confusing/bogging down more casual users

rynkk · 2022-06-07T10:09:42Z

@adehad, @studioj, I agree, not having to specify the batch_size for every request is less error-prone and more user-friendly, while the dict-config allows the required configurability.

I implemented the requested changes with some addition:
It is possible to set a Resource's mapped batch-size to None, which means, that no batching on our side is done, but instead we let the JIRA-backend handle it.

I also adjusted the tests. Seems like the test for _fetch_pages is starting to look a bit bloated. Fine for now?

_{Jannik Meinecke (jannik.meinecke@mercedes-benz.com) on behalf of MBition GmbH.

Provider Information}

adehad

Looking great!

fetch_pages() and the way we handle async needs its own refactoring at some point so don't worry about it for now.

tests/tests.py

jira/client.py

tests/tests.py

jira/client.py

adehad · 2022-06-07T13:25:33Z

@rynkk you should also rebase/merge the latest main if you haven't already, as we've fixed some build problems

___ Jannik Meinecke <jannik.meinecke@mercedes-benz.com> on behalf of MBition GmbH. https://github.com/mercedes-benz/foss/blob/master/PROVIDER_INFORMATION.md

rynkk · 2022-06-07T17:14:31Z

Changes implemented, some modifications of my own, as well. Tests pass, docs do not. Some issue with not being able to map ResourceType. Not sure what the cause is.

_{Jannik Meinecke (jannik.meinecke@mercedes-benz.com) on behalf of MBition GmbH.

Provider Information}

___ Jannik Meinecke <jannik.meinecke@mercedes-benz.com> on behalf of MBition GmbH. https://github.com/mercedes-benz/foss/blob/master/PROVIDER_INFORMATION.md

adehad

Good stuff, almost ready, let's see if we can fix the build

jira/client.py

tests/tests.py

jira/client.py

--- Jannik Meinecke (<jannik.meinecke@mercedes-benz.com>) on behalf of MBition GmbH. [Provider Information](https://github.com/mercedes-benz/foss/blob/master/PROVIDER_INFORMATION.md)

adehad

Just 1 change and then I'm happy, @studioj any further thoughts?

jira/client.py

tests/tests.py

--- Jannik Meinecke (<jannik.meinecke@mercedes-benz.com>) on behalf of MBition GmbH. [Provider Information](https://github.com/mercedes-benz/foss/blob/master/PROVIDER_INFORMATION.md)

rynkk · 2022-06-09T08:36:31Z

Issue-default removed, tests & docs adjusted :-)

_{Jannik Meinecke (jannik.meinecke@mercedes-benz.com) on behalf of MBition GmbH.

Provider Information}

adehad

Awesome, looks good to me, will wait a bit to see if @studioj has any comments.

Thanks for pushing through with this.

adehad · 2022-06-09T09:04:46Z

jira/client.py

+              ``{Issue: 500, Resources: None}`` will make :py:meth:`search_issues` query Issues in batches of 500, while
+              every other item type's batch-size will be controlled by the backend. (Default: None)


really excellent documentation here

studioj

seems pretty thourough although I'm counting on @adehad for the details :P

studioj · 2022-06-09T19:11:53Z

jira/client.py

@@ -716,6 +730,8 @@ def _fetch_pages(
            page_params["startAt"] = startAt
        if maxResults:
            page_params["maxResults"] = maxResults
+        elif batch_size := self._get_batch_size(item_type):


is this the first walrus operator we're using?
somebody has to go first :)
I must admit I'm not used to it yet

It's an honor (for some). Just have to be very careful about braces when using it..

___ Jannik Meinecke (<jannik.meinecke@mercedes-benz.com>) on behalf of MBition GmbH. [Provider Information](https://github.com/mercedes-benz/foss/blob/master/PROVIDER_INFORMATION.md)

rynkk · 2022-06-10T13:44:56Z

Found typos in the default_batch_size-docstring at the last glance

_{Jannik Meinecke (jannik.meinecke@mercedes-benz.com) on behalf of MBition GmbH.

Provider Information}

rynkk · 2022-06-21T17:15:22Z

Hi,
do you need anything else to get this PR done or are you just a bit busy right now?

_{Jannik Meinecke (jannik.meinecke@mercedes-benz.com) on behalf of MBition GmbH.

Provider Information}

adehad · 2022-06-26T11:58:13Z

Sorry @rynkk haven't had much free time, am having a bit more now so will try and push this through

adehad · 2022-06-28T21:34:40Z

Thanks again for pushing through with this

rynkk added 2 commits June 3, 2022 16:05

Add batch_size argument to fetch_pages and dependant methods

f231601

___ Jannik Meinecke <jannik.meinecke@mercedes-benz.com> on behalf of MBition GmbH. https://github.com/mercedes-benz/foss/blob/master/PROVIDER_INFORMATION.md

Fix possible crash in 'search_assignable_users_for_issues'

855f71d

___ Jannik Meinecke <jannik.meinecke@mercedes-benz.com> on behalf of MBition GmbH. https://github.com/mercedes-benz/foss/blob/master/PROVIDER_INFORMATION.md

rynkk commented Jun 3, 2022

View reviewed changes

jira/client.py Show resolved Hide resolved

rynkk commented Jun 3, 2022

View reviewed changes

jira/client.py Outdated Show resolved Hide resolved

adehad requested changes Jun 5, 2022

View reviewed changes

adehad mentioned this pull request Jun 5, 2022

Allow setting page_size with unlimited paging #894

Closed

adehad added the feature label Jun 5, 2022

Replace method-bound with instance-bound batch-size

257149d

adehad requested changes Jun 7, 2022

View reviewed changes

tests/tests.py Outdated Show resolved Hide resolved

jira/client.py Show resolved Hide resolved

jira/client.py Outdated Show resolved Hide resolved

jira/client.py Show resolved Hide resolved

tests/tests.py Outdated Show resolved Hide resolved

jira/client.py Outdated Show resolved Hide resolved

rynkk added 3 commits June 7, 2022 18:40

Move shared fixture to conftest.py

bd5650f

___ Jannik Meinecke <jannik.meinecke@mercedes-benz.com> on behalf of MBition GmbH. https://github.com/mercedes-benz/foss/blob/master/PROVIDER_INFORMATION.md

Add docs, fix tests, and shallow-copy bug

2bc01d8

___ Jannik Meinecke <jannik.meinecke@mercedes-benz.com> on behalf of MBition GmbH. https://github.com/mercedes-benz/foss/blob/master/PROVIDER_INFORMATION.md

Merge remote-tracking branch 'jira_base/main' into main

4fa30cc

Fix typehints and docs

804221c

___ Jannik Meinecke <jannik.meinecke@mercedes-benz.com> on behalf of MBition GmbH. https://github.com/mercedes-benz/foss/blob/master/PROVIDER_INFORMATION.md

adehad reviewed Jun 8, 2022

View reviewed changes

jira/client.py Outdated Show resolved Hide resolved

jira/client.py Outdated Show resolved Hide resolved

jira/client.py Outdated Show resolved Hide resolved

tests/tests.py Outdated Show resolved Hide resolved

adehad reviewed Jun 8, 2022

View reviewed changes

jira/client.py Outdated Show resolved Hide resolved

jira/client.py Outdated Show resolved Hide resolved

Change TypeVar to sphinx-supported typehint

ac28dea

--- Jannik Meinecke (<jannik.meinecke@mercedes-benz.com>) on behalf of MBition GmbH. [Provider Information](https://github.com/mercedes-benz/foss/blob/master/PROVIDER_INFORMATION.md)

adehad requested changes Jun 8, 2022

View reviewed changes

jira/client.py Show resolved Hide resolved

jira/client.py Outdated Show resolved Hide resolved

adehad reviewed Jun 8, 2022

View reviewed changes

tests/tests.py Show resolved Hide resolved

Remove default batch-size for Issue-Resource

c40b32c

--- Jannik Meinecke (<jannik.meinecke@mercedes-benz.com>) on behalf of MBition GmbH. [Provider Information](https://github.com/mercedes-benz/foss/blob/master/PROVIDER_INFORMATION.md)

adehad previously approved these changes Jun 9, 2022

View reviewed changes

studioj previously approved these changes Jun 9, 2022

View reviewed changes

rynkk dismissed stale reviews from studioj and adehad via e997654 June 10, 2022 07:53

Fix typos in JIRA's default_batch_sizes docstring

cf8014f

___ Jannik Meinecke (<jannik.meinecke@mercedes-benz.com>) on behalf of MBition GmbH. [Provider Information](https://github.com/mercedes-benz/foss/blob/master/PROVIDER_INFORMATION.md)

rynkk force-pushed the main branch from e997654 to cf8014f Compare June 10, 2022 07:55

rynkk requested review from adehad and studioj June 10, 2022 13:44

adehad approved these changes Jun 10, 2022

View reviewed changes

rynkk mentioned this pull request Jun 21, 2022

bug: config reading options editing JIRA.DEFAULT_OPTIONS #1402

Open

adehad mentioned this pull request Jun 26, 2022

fixed a case for search_assignable_users_for_issues where username an… #1401

Closed

adehad changed the title ~~Add batch_size argument to JIRA._fetch_pages and dependant methods~~ Configurable batch size for JIRA._fetch_pages() and dependant methods Jun 28, 2022

adehad merged commit 60b503a into pycontribs:main Jun 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configurable batch size for `JIRA._fetch_pages()` and dependant methods #1394

Configurable batch size for `JIRA._fetch_pages()` and dependant methods #1394

rynkk commented Jun 3, 2022 •

edited

Loading

rynkk commented Jun 3, 2022 •

edited

Loading

studioj commented Jun 4, 2022 •

edited

Loading

studioj commented Jun 5, 2022 •

edited

Loading

adehad left a comment •

edited

Loading

rynkk commented Jun 7, 2022 •

edited

Loading

adehad left a comment

adehad commented Jun 7, 2022

rynkk commented Jun 7, 2022 •

edited

Loading

adehad left a comment

adehad left a comment

rynkk commented Jun 9, 2022 •

edited

Loading

adehad left a comment

adehad Jun 9, 2022

studioj left a comment

studioj Jun 9, 2022

rynkk Jun 10, 2022

rynkk commented Jun 10, 2022 •

edited

Loading

rynkk commented Jun 21, 2022 •

edited

Loading

adehad commented Jun 26, 2022

adehad commented Jun 28, 2022

		``{Issue: 500, Resources: None}`` will make :py:meth:`search_issues` query Issues in batches of 500, while
		every other item type's batch-size will be controlled by the backend. (Default: None)

Configurable batch size for JIRA._fetch_pages() and dependant methods #1394

Configurable batch size for JIRA._fetch_pages() and dependant methods #1394

Conversation

rynkk commented Jun 3, 2022 • edited Loading

rynkk commented Jun 3, 2022 • edited Loading

studioj commented Jun 4, 2022 • edited Loading

studioj commented Jun 5, 2022 • edited Loading

adehad left a comment • edited Loading

Choose a reason for hiding this comment

rynkk commented Jun 7, 2022 • edited Loading

adehad left a comment

Choose a reason for hiding this comment

adehad commented Jun 7, 2022

rynkk commented Jun 7, 2022 • edited Loading

adehad left a comment

Choose a reason for hiding this comment

adehad left a comment

Choose a reason for hiding this comment

rynkk commented Jun 9, 2022 • edited Loading

adehad left a comment

Choose a reason for hiding this comment

adehad Jun 9, 2022

Choose a reason for hiding this comment

studioj left a comment

Choose a reason for hiding this comment

studioj Jun 9, 2022

Choose a reason for hiding this comment

rynkk Jun 10, 2022

Choose a reason for hiding this comment

rynkk commented Jun 10, 2022 • edited Loading

rynkk commented Jun 21, 2022 • edited Loading

adehad commented Jun 26, 2022

adehad commented Jun 28, 2022

Configurable batch size for `JIRA._fetch_pages()` and dependant methods #1394

Configurable batch size for `JIRA._fetch_pages()` and dependant methods #1394

rynkk commented Jun 3, 2022 •

edited

Loading

rynkk commented Jun 3, 2022 •

edited

Loading

studioj commented Jun 4, 2022 •

edited

Loading

studioj commented Jun 5, 2022 •

edited

Loading

adehad left a comment •

edited

Loading

rynkk commented Jun 7, 2022 •

edited

Loading

rynkk commented Jun 7, 2022 •

edited

Loading

rynkk commented Jun 9, 2022 •

edited

Loading

rynkk commented Jun 10, 2022 •

edited

Loading

rynkk commented Jun 21, 2022 •

edited

Loading