Rebalance the modules to reduce complexity #298

nolar · 2020-01-24T10:29:31Z

What do these changes do?

Move classes and functions across the modules, extract new modules — in order to reduce the complexity and prepare the codebase for adding even more code.

The behaviour is not changed, only the imports.

Description

Before this PR, some modules were too large, up to 1k of lines, containing too many aspects and topics — as it has historically happened when new features were added. For example, activity execution was part of handling.py, as it was nearly a clone of handler execution. Similarly, event processing and handler invocation were there too (in handling.py) — since the times they were short and simple.

With this PR, some huge modules are split into smaller ones, and classes/functions are moved all around. For example, watch-event handling is now renamed to processing and moved to processing.py — as per event processing of the event-driven design, which it is by its nature — this releases the term handling to the callback functions, which are currently called handlers.

This is needed to make more space for adding more code (e.g. for daemons and timers) without exploding the modules beyond understanding.

See the list of commits for individual changes (they are all atomic). Briefly:

Rename watch-event "handling" routines to "processing" routines.
Extract the in-memory multi-step handling cycle to a reusable routine (used in activities, will be used in daemons/timers).
Split handling.py -> handling.py + processing.py + activities.py
Split registries.py -> registries.py + handlers.py + callbacks.py + errors.py

The latter split also removes the cyclic imports as detected by LGTM (all 16 alarms at once) — this cycle is broken by moving BaseHandler away from registries:

→ registries
- → invocation (for get_kwargs(), needed for matching callbacks)
  - → lifecycles (for LifeCycleFn, needed for Invokable definition)
    - → registries (for BaseHandler, needed for handlers kwarg)

Replaced with a regular dependency tree:

→ registries
- → invocation (for get_kwargs(), needed for matching callbacks)
  - → lifecycles (for LifeCycleFn, needed for Invokable definition)
    - → handlers (for BaseHandler, needed for handlers kwarg)
- → handlers (for all handlers dataclasses, previously located in registries)

The users should not be affected, as these are the internal changes only — unless they monkey-patch some of the Kopf's internals.

Issues/PRs

Issues: #19

Related: #315

Type of changes

Refactoring (non-breaking change which does not alter the behaviour)

Checklist

The code addresses only the mentioned problem, and this problem only
I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
If you provide code modification, please add yourself to CONTRIBUTORS.txt

In order to keep the word "handler" for the user-facing handler functions only (created with decorators). The word "processor" is taken to match with the event-driver or event-stream system design terminology.

`kopf.reactor.handling` is already overloaded with logic and entities, it is time to split it. The dividing line is the processing of the incoming watch-events vs. executing the handlers. Also, vs. invocation of arbitrary callbacks, not only the handlers, in `kopf.reactor.invocation`.

They contain a lot of copy-pasting, so it is better to keep them away from the meaningful code (handlers & registries). As they are more protocols are going to be added, separating them seems a good refactoring move to rebalance the modules. Also, they are similar to `kopf.reactor.lifecycles`, as they declare the invokable function signatures the same way.

zincr · 2020-01-24T10:29:42Z

🤖 zincr found 0 problems , 1 warning

ℹ️ Large Commits
✅ Approvals
✅ Specification
✅ Dependency Licensing

Details on how to resolve are provided below

Large Commits

Checks all commits for large additions to a single file. Large commits should be reviewed more carefully for potential copyright and licensing issues

This file contains a substantial change, please review to determine if the change comes from an external source and if there are any copyright or licensing issues to be aware of

ℹ️ kopf/reactor/handling.py had +100 lines of code changed in a single commit
Please review this file to determine the source of this change
ℹ️ kopf/reactor/processing.py had +100 lines of code changed in a single commit
Please review this file to determine the source of this change
ℹ️ kopf/reactor/registries.py had +100 lines of code changed in a single commit
Please review this file to determine the source of this change

zincr · 2020-01-24T10:29:43Z

🤖 zincr found 1 problem , 1 warning

❌ Approvals
ℹ️ Large Commits
✅ Specification
✅ Dependency Licensing

Details on how to resolve are provided below

Approvals

All proposed changes must be reviewed by project maintainers before they can be merged

Not enough people have approved this pull request - please ensure that 1 additional user, who have not contributed to this pull request approve the changes.

✅ Approved by PR author @nolar
❌ 1 additional approval needed

Large Commits

Checks all commits for large additions to a single file. Large commits should be reviewed more carefully for potential copyright and licensing issues

This file contains a substantial change, please review to determine if the change comes from an external source and if there are any copyright or licensing issues to be aware of

ℹ️ kopf/reactor/handling.py had +100 lines of code changed in a single commit
Please review this file to determine the source of this change
ℹ️ kopf/reactor/processing.py had +100 lines of code changed in a single commit
Please review this file to determine the source of this change
ℹ️ kopf/reactor/registries.py had +100 lines of code changed in a single commit
Please review this file to determine the source of this change

lgtm-com · 2020-01-24T10:38:03Z

This pull request fixes 4 alerts when merging 147f5de into f82c90b - view on LGTM.com

fixed alerts:

3 for Module-level cyclic import
1 for Unused import

lgtm-com · 2020-01-27T14:58:21Z

This pull request fixes 17 alerts when merging ac3b646 into f82c90b - view on LGTM.com

fixed alerts:

16 for Module-level cyclic import
1 for Unused import

lgtm-com · 2020-01-29T11:40:49Z

This pull request fixes 17 alerts when merging 322a562 into f82c90b - view on LGTM.com

fixed alerts:

16 for Module-level cyclic import
1 for Unused import

nolar added 9 commits January 21, 2020 17:46

Rename stream handlers to processors in watching/queueing

a7ca629

In order to keep the word "handler" for the user-facing handler functions only (created with decorators). The word "processor" is taken to match with the event-driver or event-stream system design terminology.

Rename activity_trigger() -> run_activity()

5c1fda0

Rename call_handler() -> invoke_handler() to match with invoke()

2634e53

Rename handler execution methods to mention it is done "once"

6a6a35f

Extract the in-memory "until done" handler execution reusable routine

c5cfa53

Move remaining activities -> kopf.reactor.activities

f6b4b1b

Reuse the existing typedef of FieldSpec for on.field decorators

147f5de

nolar added the refactoring Code cleanup without new features added label Jan 24, 2020

nolar added 2 commits January 27, 2020 15:27

Move error modes to its own module -- to unload registries

3fe6cf1

Move handlers to its own module -- to unload registries

ac3b646

nolar marked this pull request as ready for review January 27, 2020 15:03

nolar requested a review from aweller January 27, 2020 20:59

nolar mentioned this pull request Jan 27, 2020

adopt(V1Secret) fails #286

Open

Resolve naming conflict for handlers (module-vs-args) in states.py

322a562

samurang87 approved these changes Jan 29, 2020

View reviewed changes

nolar merged commit 3614ee4 into zalando-incubator:master Jan 29, 2020

nolar deleted the rebalance-modules-3 branch January 29, 2020 13:51

nolar mentioned this pull request Feb 20, 2020

Simplify registries: no prefixes, direct sub-registries access #315

Merged

5 tasks

kopf-archiver bot mentioned this pull request Aug 19, 2020

[PR] Rebalance the modules to reduce complexity nolar/kopf#298

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rebalance the modules to reduce complexity #298

Rebalance the modules to reduce complexity #298

nolar commented Jan 24, 2020 •

edited

Loading

zincr bot commented Jan 24, 2020 •

edited

Loading

zincr bot commented Jan 24, 2020

lgtm-com bot commented Jan 24, 2020

lgtm-com bot commented Jan 27, 2020

lgtm-com bot commented Jan 29, 2020

Rebalance the modules to reduce complexity #298

Rebalance the modules to reduce complexity #298

Conversation

nolar commented Jan 24, 2020 • edited Loading

What do these changes do?

Description

Issues/PRs

Type of changes

Checklist

zincr bot commented Jan 24, 2020 • edited Loading

🤖 zincr found 0 problems , 1 warning

Large Commits

zincr bot commented Jan 24, 2020

🤖 zincr found 1 problem , 1 warning

Approvals

Large Commits

lgtm-com bot commented Jan 24, 2020

lgtm-com bot commented Jan 27, 2020

lgtm-com bot commented Jan 29, 2020

nolar commented Jan 24, 2020 •

edited

Loading

zincr bot commented Jan 24, 2020 •

edited

Loading