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.

Sign up for GitHub

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

Jump to bottom

Refined Interface of EventSource and EventSourceManager #597

Merged
merged 45 commits into from
Oct 13, 2021
Merged

Refined Interface of EventSource and EventSourceManager #597

merged 45 commits into from
Oct 13, 2021

Conversation

csviri
Copy link
Collaborator

@csviri csviri commented Oct 12, 2021

No description provided.

csviri and others added 30 commits September 30, 2021 17:02
@csviri
WIP
898e2c2
@csviri
Addressing Custom Resource by Name and Namespace refactor + Informer …
be4e5cf 
…Cache WIP
@csviri
Build is fixed, (test failing)
b2ab4b9
@csviri
Test fixes
9145b52
@csviri
Merge branch 'master' into informer-creventsource
4ef27bf 
# Conflicts:
#	operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/Event.java
@csviri
minor update
34cc2a1
@csviri
EventSourceManager small fix
4b15974
@csviri
Merge branch 'access-event-source-manager' into informer-creventsource
77033e6
@csviri
Unit tests fixed
e1b5926
@csviri
fix: DefaultEventHandler init from EventSourceManager
e7d1b99
@csviri
fix: custom resource selector test improvement
a1f92c6
@csviri
fix: wip test imrpovements
e928a3e
@csviri
fix: test improvements
f35a340
@csviri
fix: further improvements
5d5817e
@csviri
Merge branch 'v2' into informer-creventsource
544ce35 
# Conflicts:
#	operator-framework-core/src/main/java/io/javaoperatorsdk/operator/Operator.java
#	operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java
@csviri
fix: build
75ad7d2
@csviri
feature: add mvn jar to gitignore
7012fb3
@csviri
Exposing CustomResourceEventSource and informers
3746122
@csviri
fix: cleanup
f0f2e91
@csviri
fix: remove caching optimization since it not possible anymore with i…
6638a48 
…nformer
@csviri
fix: formatting
1d786ef
@metacosm
refactor: make name/namespace final
a5343b7
@csviri
feature: Simple label selector support
9e0430a
@csviri
Merge branch 'informer-creventsource' of github.com:java-operator-sdk…
7f48b9a 
…/java-operator-sdk into informer-creventsource
@csviri
fix: formatting
0aa29a1
@csviri
fix: code inspection reports
3ad2fc5
@csviri
Merge branch 'v2' into informer-creventsource
86b8185 
# Conflicts:
#	operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java
@csviri
fix: merge from v2
92d3ed3
@csviri
fix: removed most deprecated apis
d194d25
@iocanel @metacosm
chore: renaming vars named k8sClient to kubernetsClient
2e8f219
dependabot bot and others added 13 commits October 8, 2021 09:08
@dependabot
chore(deps): bump jandex-maven-plugin from 1.1.1 to 1.2.1 (#592)
9ebfff0 
Bumps [jandex-maven-plugin](https://github.com/wildfly/jandex-maven-plugin) from 1.1.1 to 1.2.1.
- [Release notes](https://github.com/wildfly/jandex-maven-plugin/releases)
- [Commits](wildfly/jandex-maven-plugin@1.1.1...1.2.1)

---
updated-dependencies:
- dependency-name: org.jboss.jandex:jandex-maven-plugin
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@dependabot
chore(deps-dev): bump mockito-core from 3.12.4 to 4.0.0 (#591)
f1f3867 
Bumps [mockito-core](https://github.com/mockito/mockito) from 3.12.4 to 4.0.0.
- [Release notes](https://github.com/mockito/mockito/releases)
- [Commits](mockito/mockito@v3.12.4...v4.0.0)

---
updated-dependencies:
- dependency-name: org.mockito:mockito-core
  dependency-type: direct:development
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@csviri @metacosm
feature: Build PR on v2
b99ff10
@metacosm
chore(ci): use Java 17
326f82f
@metacosm
chore(ci): use only Temurin distribution
98e3def
@csviri
fix: Updated informer mapping to CustomResourceID
a03cfb9
@lburgazzoli
chore: add generics to PostExecutionControl to reduce IDEs noise (#594)
746f0c1
@lburgazzoli
chore: polish the junit5 extension (#593)
bb6c00e
@csviri
fix: EventSourceManager API wip
80e8238
@csviri
fix: code review fixes
bbecf4c
@csviri
Merge branch 'master' into informer-creventsource
26b1c3e
@csviri
Merge branch 'informer-creventsource' into event-source-register-as-list
f350f46
@csviri
fix: improvements of Event Source related APIs
d3e2469
@csviri csviri linked an issue Oct 12, 2021 that may be closed by this pull request
Registering event sources in EventSourceManager without a name #566
Closed
@csviri csviri self-assigned this Oct 12, 2021
@csviri
Copy link
Collaborator Author

csviri commented Oct 12, 2021

Pls review after this is merged: #581

@snowdrop-bot snowdrop-bot mentioned this pull request Oct 12, 2021
Closed
@csviri csviri linked an issue Oct 12, 2021 that may be closed by this pull request
EventSourceManager interface #405
Closed
@metacosm
Copy link
Collaborator

Can you please rebase now that the informer PR has been merged?

@csviri
Merge branch 'v2' into event-source-register-as-list
974c0eb 
# Conflicts:
#	operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java
#	operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java
#	operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSource.java
#	operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSourceManager.java
#	operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/TimerEventSource.java
#	operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java
#	operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManagerTest.java
#	operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/TimerEventSourceTest.java
#	operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/AnnotationConfiguration.java
@csviri
Copy link
Collaborator Author

csviri commented Oct 12, 2021

Can you please rebase now that the informer PR has been merged?

yep, done

metacosm
metacosm requested changes Oct 12, 2021
View reviewed changes
...re/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java Outdated
private static final Logger log = LoggerFactory.getLogger(DefaultEventSourceManager.class);

private final ReentrantLock lock = new ReentrantLock();
private final Map<String, EventSource> eventSources = new ConcurrentHashMap<>();
private final List<EventSource> eventSources = Collections.synchronizedList(new ArrayList<>());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't it be a Set, actually? We don't need the ordering but we'd rather have only one instead of the EventSource in the collection…

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, will change it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed.

...work-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSourceManager.java
* @param eventSource the {@link EventSource} to register
* @throws IllegalStateException if an {@link EventSource} with the same name is already
* registered.
* @throws OperatorException if an error occurred during the registration process
*/
void registerEventSource(String name, EventSource eventSource)
void registerEventSource(EventSource eventSource)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we also make it possible to remove an EventSource?

Copy link
Collaborator Author

@csviri csviri Oct 13, 2021 •
edited

Choose a reason for hiding this comment

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

I think it should not. So were thinking how to handle some dynamic registration and de-registration of event sources. But that would make sense only per custom resource. Since we want to cover events for all related custom resources which are in differenet lifecycle state. So deregistering an event source will mean that new custom resources would not receive events about related dependent resource anymore, even the custom resource was just created.

I see it rather this way: notifications for event sources about custom resource lifecycle, what we already partially cover:

  • now we have cleanupForCustomResource when a custom resource delete,
  • similarly we could have an event that customResourceCreated. So event source could execute some related logic. Like register a webhook or etc. This would make it very general. But did not want to do it until we have some actual request. We already have for the cleanup.

@csviri csviri merged commit 8e9300e into v2 Oct 13, 2021
@csviri csviri deleted the event-source-register-as-list branch October 13, 2021 13:15
metacosm added a commit that referenced this pull request Oct 28, 2021
@csviri @metacosm
@iocanel @dependabot @lburgazzoli
Refined Interface of EventSource and EventSourceManager (#597)
1c9f257 
* WIP

* Addressing Custom Resource by Name and Namespace refactor + Informer Cache WIP

* fix: DefaultEventHandler init from EventSourceManager

* fix: custom resource selector test improvement

* fix: wip test imrpovements

* fix: test improvements

* fix: further improvements

* fix: build

* feature: add mvn jar to gitignore

* Exposing CustomResourceEventSource and informers

* fix: cleanup

* fix: remove caching optimization since it not possible anymore with informer

* fix:  formatting

* refactor: make name/namespace final

* feature: Simple label selector support

* fix: formatting

* fix: code inspection reports

* fix: merge from v2

* fix: removed most deprecated apis

* chore: renaming vars named k8sClient to kubernetsClient

* chore(deps): bump jandex-maven-plugin from 1.1.1 to 1.2.1 (#592)

Bumps [jandex-maven-plugin](https://github.com/wildfly/jandex-maven-plugin) from 1.1.1 to 1.2.1.
- [Release notes](https://github.com/wildfly/jandex-maven-plugin/releases)
- [Commits](wildfly/jandex-maven-plugin@1.1.1...1.2.1)

---
updated-dependencies:
- dependency-name: org.jboss.jandex:jandex-maven-plugin
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* chore(deps-dev): bump mockito-core from 3.12.4 to 4.0.0 (#591)

Bumps [mockito-core](https://github.com/mockito/mockito) from 3.12.4 to 4.0.0.
- [Release notes](https://github.com/mockito/mockito/releases)
- [Commits](mockito/mockito@v3.12.4...v4.0.0)

---
updated-dependencies:
- dependency-name: org.mockito:mockito-core
  dependency-type: direct:development
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* feature: Build PR on v2

* chore(ci): use Java 17

* chore(ci): use only Temurin distribution

* fix: Updated informer mapping to CustomResourceID

* chore: add generics to PostExecutionControl to reduce IDEs noise (#594)

* chore: polish the junit5 extension (#593)

* fix: EventSourceManager API wip

* fix: code review fixes

* fix: improvements of Event Source related APIs

* fix: remarks from code review

Co-authored-by: Chris Laprun <metacosm@gmail.com>
Co-authored-by: Ioannis Canellos <iocanel@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Luca Burgazzoli <lburgazzoli@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@metacosm metacosm metacosm approved these changes

@lburgazzoli lburgazzoli Awaiting requested review from lburgazzoli

Assignees

@csviri csviri

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Registering event sources in EventSourceManager without a name EventSourceManager interface
4 participants
@csviri @metacosm @iocanel @lburgazzoli