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

Add support for Coordinator to Equinox Configuration Admin #533

Merged
merged 1 commit into from
Feb 18, 2024

Conversation

laeubi
Copy link
Member

@laeubi laeubi commented Feb 16, 2024

Chapter 11 of the Configuration Admin Service Specification mandates the support of the Coordinator service but currently Equinox fails the TCK tests in that area.

This adds support for participation in the coordinator for the Equinox Configuration Admin.

Copy link

github-actions bot commented Feb 16, 2024

Test Results

 1 files   -    26   1 suites   - 26   4s ⏱️ - 11m 33s
14 tests  - 2 156  14 ✅  - 2 110  0 💤  - 46  0 ❌ ±0 
14 runs   - 2 214  14 ✅  - 2 168  0 💤  - 46  0 ❌ ±0 

Results for commit fdd9c52. ± Comparison against base commit 841faf8.

This pull request removes 2156 tests.
org.eclipse.core.runtime.tests.FileLocatorTest ‑ testFileLocatorFind
org.eclipse.core.runtime.tests.FileLocatorTest ‑ testFileLocatorGetBundleFile01
org.eclipse.core.runtime.tests.FileLocatorTest ‑ testFileLocatorGetBundleFile02
org.eclipse.equinox.bidi.internal.tests.StructuredTextExtensibilityTest ‑ testOtherContributions
org.eclipse.equinox.bidi.internal.tests.StructuredTextExtensionsTest ‑ testDefaultExtensions
org.eclipse.equinox.bidi.internal.tests.StructuredTextExtensionsTest ‑ testTestExtensions
org.eclipse.equinox.bidi.internal.tests.StructuredTextFullToLeanTest ‑ testFullToLean
org.eclipse.equinox.bidi.internal.tests.StructuredTextMathTest ‑ testRTLarithmetic
org.eclipse.equinox.bidi.internal.tests.StructuredTextMethodsTest ‑ testMethods
org.eclipse.equinox.bidi.internal.tests.StructuredTextProcessorTest ‑ testStructuredTextProcessor
…

♻️ This comment has been updated with latest results.

@laeubi laeubi force-pushed the add_cm_coordinator_support branch 6 times, most recently from 15ed6d8 to b69693b Compare February 18, 2024 12:37
@laeubi laeubi linked an issue Feb 18, 2024 that may be closed by this pull request
@laeubi laeubi force-pushed the add_cm_coordinator_support branch 8 times, most recently from 7cd9537 to b177930 Compare February 18, 2024 17:34
Chapter 11 of the Configuration Admin Service Specification mandates the
support of the Coordinator service but currently Equinox fails the TCK
tests in that area.

This adds support for participation in the coordinator for the Equinox
Configuration Admin.
@laeubi
Copy link
Member Author

laeubi commented Feb 18, 2024

I could get it now down to one failing test in the TCK but this is tricky if not fully specified, sadly the chapter in the Spec is rather short:

The Coordinator Service Specification defines a mechanism for multiple parties to collaborate on a common task without a priori knowledge of who will collaborate in that task. The Configuration Admin service must participate in such scenarios to coordinate with provisioning or configuration tasks.

If configurations are created, updated or deleted and an implicit coordination exists, the Configuration Admin service must delay notifications until the coordination terminates. However the configuration changes must be persisted immediately. Updating a Managed Service or Managed Service Factory and informing asynchronous listeners is delayed until the coordination terminates, regardless of whether the coordination fails or terminates regularly. Registered synchronous listeners will be informed immediately when the change happens regardless of a coordination.

The failing test works that way:

  1. registers a ManagedService and receives an update with properties = null
  2. The starts a coordination update it and delete it afterwards
  3. the is asserts that update was only called one

The assert fails for equinox because two calls to update with properties = null are recorded, but from the spec text it is unclear that this is wrong as it only says the update must be delayed, but not that it must detect that the service actually is already contain the state it is now updated to.

Especially the chapter 104.7.4 Updating a Configuration mentions that a ConfigAdmin might blindly update a ManagedService if no special action is taken here, so I think this is maybe even covered by the spec.

@laeubi laeubi merged commit 906f6d6 into eclipse-equinox:master Feb 18, 2024
24 of 28 checks passed
@iloveeclipse
Copy link
Member

@laeubi : not sure you've missed that we are in RC1 phase, and all commits require a review from other committer before merge?

@merks
Copy link
Contributor

merks commented Feb 18, 2024

The email suggests the freeze starts the 21st

https://www.eclipse.org/lists/equinox-dev/msg09560.html

I haven’t seen PRs with freeze failures yet either. E.g., all the aggregator PRs did not indicate there is a freeze yet.

Perhaps there is some confusion or miscommunication?

@laeubi
Copy link
Member Author

laeubi commented Feb 19, 2024

@laeubi : not sure you've missed that we are in RC1 phase, and all commits require a review from other committer before merge?

Thtas possible its still a bit confusing sometimes but as the freeze check does not complains I thought that it is fine calendar says RC1 stabilization start February 20 6am

@iloveeclipse
Copy link
Member

I think it is obvious, that all changes merged after M3 are going for RC1?
So independently when the RC1 is planned all changes after M3 require extra approval, review etc.

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.

org.eclipse.equinox.cm seem to fails some TCKs
3 participants