improve integration doc #9322
Open
improve integration doc #9322
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for your pull request! This pull request does not follow the contribution rules. Could you have a look? ❌ All commit messages should start with a JIRA issue key matching pattern › This message was automatically generated. |
| @@ -35,17 +35,15 @@ What's interesting is the `ServiceRegistry` and the pluggable swapping of the di | |||
| The basic requirement for a `Service` is to implement the marker interface `org.hibernate.service.Service`. | |||
| Hibernate uses this internally for some basic type safety. | |||
|
|
|||
| The `Service` can also implement a number of optional life-cycle related contracts: | |||
| The `Service` can also implement a number of optional lifecycle related contracts: | |||
There was a problem hiding this comment.
change this to maintain consistency. lifecycle is used elsewhere
|
|
||
| `org.hibernate.service.spi.Startable`:: | ||
| allows the `Service` impl to be notified that it is being started and about to be put into use. | ||
| `org.hibernate.service.spi.Stoppable`:: | ||
| allows the `Service` impl to be notified that it is being stopped and will be removed from use. | ||
| `org.hibernate.service.spi.ServiceRegistryAwareService`:: | ||
| allows the `Service` to be injected with a reference to the registry that is managing it. See <<services-dependencies>> for more details. | ||
| `org.hibernate.service.spi.Manageable`:: | ||
| marks the `Service` as manageable in JMX provided the JMX integration is enabled. This feature is still incomplete. |
There was a problem hiding this comment.
The above stuff has been absent from the codebase
| @@ -118,12 +116,12 @@ Each specific type of registry defines its own `ServiceInitiator` specialization | |||
| === Types of ServiceRegistries | |||
|
|
|||
| Currently Hibernate utilizes three different `ServiceRegistry` implementations forming a hierarchy. | |||
| Each type is a specialization for the purpose of type-safety, but they add no new functionality. | |||
| Each type is a specialization for the purpose of type safety, but they add no new functionality. | |||
There was a problem hiding this comment.
for consistency reason. type safety is used more often elsewhere.
| @@ -1,4 +1,4 @@ | |||
| public interface EventPublishingService extends Service { | |||
|
|
|||
| public void publish(Event theEvent); | |||
| void publish(Event theEvent); | |||
There was a problem hiding this comment.
I believe we don't need to specify public for interface method
| .get( JMS_CONNECTION_FACTORY_NAME_SETTING ); | ||
| this.destinationName = configurationValues | ||
| this.destinationName = (String) configurationValues | ||
| .get( JMS_DESTINATION_NAME_SETTING ); |
There was a problem hiding this comment.
I added generics usage as much as possible in this PR for using Map alone seems sloppy
| @@ -1,4 +1,4 @@ | |||
| public class LatestAndGreatestConnectionProviderImplContributor1 | |||
| public class LatestAndGreatestConnectionProviderImplContributor | |||
| implements ServiceContributor { | |||
There was a problem hiding this comment.
Originally the example code snippets were prepared for two flavours:
- LatestAndGreatestConnectionProviderImplContributor1
- LatestAndGreatestConnectionProviderImplContributor2
one of the reasons is two meta-inf files were provided so using number suffix to differentiate between them:
- ex2-meta-inf
- ex3-meta-inf
However, the ex3-meta-inf was not used at all so it is deleted in this PR. Then it seems no such confusing suffix is needed.
| selector.registerStrategyImplementor( | ||
| ConnectionProvider.class, | ||
| "lag", | ||
| LatestAndGreatestConnectionProviderImpl.class | ||
| ); | ||
| } |
There was a problem hiding this comment.
this code snippet is too sloppy
There was a problem hiding this comment.
Probably should be requireService() instead of getService() if we're being picky.
gavinking
reviewed
Nov 24, 2024
| + | ||
| By default, injected services are considered required, that is the start up will fail if a named dependent `Service` is missing. | ||
| If the `Service` to be injected is optional, the required attribute of the `@InjectService` annotation should be declared as `false` (default is `true`). | ||
| By default, injected services are considered required, that is the startup will fail if a named dependent `Service` is missing. |
There was a problem hiding this comment.
Suggested change
| By default, injected services are considered required, that is the startup will fail if a named dependent `Service` is missing. | |
| By default, injected services are considered required, that is, the startup will fail if a named dependent `Service` is missing. |
gavinking
reviewed
Nov 24, 2024
| @@ -58,10 +56,10 @@ Services are allowed to declare dependencies on other services using either of t | |||
| Any method on the `Service` implementation class accepting a single parameter and annotated with `@InjectService` is considered requesting injection of another service. | |||
| + | |||
| By default, the type of the method parameter is expected to be the `Service` role to be injected. | |||
| If the parameter type is different than the `Service` role, the serviceRole attribute of the `@InjectService` annotation should be used to explicitly name the role. | |||
| If the parameter type is different than the `Service` role, the `serviceRole` attribute of the `@InjectService` annotation should be used to explicitly name the role. | |||
There was a problem hiding this comment.
Suggested change
| If the parameter type is different than the `Service` role, the `serviceRole` attribute of the `@InjectService` annotation should be used to explicitly name the role. | |
| If the parameter type is different to the `Service` role, the `serviceRole` attribute of the `@InjectService` annotation should be used to explicitly name the role. |
gavinking
reviewed
Nov 24, 2024
|
|
||
| ===== `IntegratorService` | ||
|
|
||
| The `Service` role for this `Service` is `org.hibernate.integrator.spi.IntegratorService.` | ||
| Applications, third-party integrators and others all need to integrate with Hibernate. Historically this used to require something (usually the application) to coordinate registering the pieces of each integration needed on behalf of each integration. The `org.hibernate.integrator.spi.Integrator` contract formalized this "integration SPI". The IntegratorService manages all known integrators. | ||
| Applications, third-party integrators and others all need to integrate with Hibernate. Historically this used to require something (usually the application) to coordinate registering the pieces of each integration needed on behalf of each integration. The `org.hibernate.integrator.spi.Integrator` contract formalized this "integration SPI". The `IntegratorService` manages all known integrators. |
There was a problem hiding this comment.
Suggested change
| Applications, third-party integrators and others all need to integrate with Hibernate. Historically this used to require something (usually the application) to coordinate registering the pieces of each integration needed on behalf of each integration. The `org.hibernate.integrator.spi.Integrator` contract formalized this "integration SPI". The `IntegratorService` manages all known integrators. | |
| Applications, third-party integrators, and others all need to integrate with Hibernate. Historically this used to require something (usually the application) to coordinate registering the pieces of each integration needed on behalf of each integration. The `org.hibernate.integrator.spi.Integrator` contract formalized this "integration SPI". The `IntegratorService` manages all known integrators. |
Oxford comma.
gavinking
reviewed
Nov 24, 2024
|
|
||
| // if we wanted to allow other strategies (e.g. a jms | ||
| // queue publisher) we might also register short names | ||
| // queue publisher) we might also register short-names |
There was a problem hiding this comment.
Hyphenating "short names" is just weird. You don't need a hyphen between an adjective and a noun.
A hyphen would be used if this were a compound adjective, but it's not. So if there are other occurrences of "short-name", they should be fixed by removing the hyphen.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Recently working on Hibernate Integration stuff in MongoDB. After carefully reading the excellent Integration doc, it seems there are some minor defects that could be fixed.
As normal, many might be false positives. I'll leave some comments to explain the reasons behind some changes.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.