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

Remove 'split packages' #7246

Closed
planetf1 opened this issue Dec 13, 2022 · 5 comments
Closed

Remove 'split packages' #7246

planetf1 opened this issue Dec 13, 2022 · 5 comments
Assignees
Labels
enhancement New feature or request triage New bug/issue which needs checking & assigning

Comments

@planetf1
Copy link
Member

planetf1 commented Dec 13, 2022

Existing/related issue?

#2377

Please describe the new behavior that that will improve Egeria

With 'Java Modules' (in Java 9 and above), 'split packages' are not allowed.

This is where the same package name is found in two different modules (=jars).
See https://www.logicbig.com/tutorials/core-java-tutorial/modules/split-packages.html

Generally in Egeria we adhere to this principle, however a few anomalies were found when working on the referenced issue #2377

In the 'demonstration' PR #7245 the modified directories needed were:

open-metadata-implementation/adapters/open-connectors/rest-client-connectors/rest-client-factory/src/main/java/org/odpi/openmetadata/adapters/connectors/restclients >>>> open-metadata-implementation/adapters/open-connectors/rest-client-connectors/rest-client-factory/src/main/java/org/odpi/openmetadata/adapters/connectors/restclients/factory
open-metadata-implementation/admin-services/admin-services-api/src/main/java/org/odpi/openmetadata/adminservices >>>> open-metadata-implementation/admin-services/admin-services-api/src/main/java/org/odpi/openmetadata/adminservices/api
open-metadata-implementation/admin-services/admin-services-api/src/main/java/org/odpi/openmetadata/adminservices/configuration/properties >>>> open-metadata-implementation/admin-services/admin-services-api/src/main/java/org/odpi/openmetadata/adminservices/api/configuration/properties
open-metadata-implementation/admin-services/admin-services-api/src/main/java/org/odpi/openmetadata/adminservices/configuration/registration >>>> open-metadata-implementation/admin-services/admin-services-api/src/main/java/org/odpi/openmetadata/adminservices/api/configuration/registration
open-metadata-implementation/admin-services/admin-services-api/src/main/java/org/odpi/openmetadata/adminservices/ffdc >>>> open-metadata-implementation/admin-services/admin-services-api/src/main/java/org/odpi/openmetadata/adminservices/api/ffdc
open-metadata-implementation/admin-services/admin-services-api/src/main/java/org/odpi/openmetadata/adminservices/ffdc/exception >>>> open-metadata-implementation/admin-services/admin-services-api/src/main/java/org/odpi/openmetadata/adminservices/api/ffdc/exception
open-metadata-implementation/admin-services/admin-services-api/src/main/java/org/odpi/openmetadata/adminservices/properties >>>> open-metadata-implementation/admin-services/admin-services-api/src/main/java/org/odpi/openmetadata/adminservices/api/properties
open-metadata-implementation/admin-services/admin-services-api/src/main/java/org/odpi/openmetadata/adminservices/rest >>>> open-metadata-implementation/admin-services/admin-services-api/src/main/java/org/odpi/openmetadata/adminservices/api/rest
open-metadata-implementation/admin-services/admin-services-api/src/main/java/org/odpi/openmetadata/adminservices/store >>>> open-metadata-implementation/admin-services/admin-services-api/src/main/java/org/odpi/openmetadata/adminservices/api/store
open-metadata-implementation/admin-services/admin-services-api/src/test/java/org/odpi/openmetadata/adminservices/configuration/properties >>>> open-metadata-implementation/admin-services/admin-services-api/src/test/java/org/odpi/openmetadata/adminservices/api/configuration/properties
open-metadata-implementation/admin-services/admin-services-api/src/test/java/org/odpi/openmetadata/adminservices/configuration/registration >>>> open-metadata-implementation/admin-services/admin-services-api/src/test/java/org/odpi/openmetadata/adminservices/api/configuration/registration
open-metadata-implementation/admin-services/admin-services-api/src/test/java/org/odpi/openmetadata/adminservices/ffdc >>>> open-metadata-implementation/admin-services/admin-services-api/src/test/java/org/odpi/openmetadata/adminservices/api/ffdc
open-metadata-implementation/admin-services/admin-services-api/src/test/java/org/odpi/openmetadata/adminservices/rest >>>> open-metadata-implementation/admin-services/admin-services-api/src/test/java/org/odpi/openmetadata/adminservices/api/rest
open-metadata-implementation/admin-services/admin-services-registration/src/main/java/org/odpi/openmetadata/adminservices/configuration/registration >>>> open-metadata-implementation/admin-services/admin-services-registration/src/main/java/org/odpi/openmetadata/adminservices/configuration/registration/admin
open-metadata-implementation/view-services/dino-view/dino-view-spring/src/main/java/org/odpi/openmetadata/viewservices/dino/server >>>> open-metadata-implementation/view-services/dino-view/dino-view-spring/src/main/java/org/odpi/openmetadata/viewservices/dino/spring
open-metadata-implementation/view-services/rex-view/rex-view-spring/src/main/java/org/odpi/openmetadata/viewservices/rex/server >>>> open-metadata-implementation/view-services/rex-view/rex-view-spring/src/main/java/org/odpi/openmetadata/viewservices/rex/spring
open-metadata-implementation/view-services/tex-view/tex-view-spring/src/main/java/org/odpi/openmetadata/viewservices/tex/server >>>> open-metadata-implementation/view-services/tex-view/tex-view-spring/src/main/java/org/odpi/openmetadata/viewservices/tex/spring

These names can be modified, but the affected packages are:

org.odpi.openmetadata.adapters.connectors.restclients

  • rest-client-factory
  • rest-client-connectors-api

org.odpi.openmetadata.adminservices

  • admin-services-api
  • admin-services-server

org.odpi.openmetadata.adminservices.configuration.registration

  • admin-services-api
  • admin-services-registration

org.odpi.openmetadata.viewservices.dino.server

  • dino-view-server
  • dino-view-spring

org.odpi.openmetadata.viewservices.rex.server

  • rex-view-server
  • rex-view-spring

org.odpi.openmetadata.viewservices.tex.server

  • tex-view-server
  • rex-view-spring

Alternatives

No response

Any Further Information?

No response

Would you be prepared to be assigned this issue to work on?

No response

@planetf1 planetf1 added enhancement New feature or request triage New bug/issue which needs checking & assigning labels Dec 13, 2022
@planetf1
Copy link
Member Author

The referenced PR changes more than required (investigation work needed to resolve issues), so the renames there will be undone.

  • rex/tex/dino - simple rename the package in the spring modules to org.odpi.openmetadata.viewservices.tex.spring etc
  • restclients - suggest changing the package used in the factory to org.odpi.openmetadata.adapters.connectors.restclients.factory
  • admin services - initially i renamed the 'api' packages to qualify, but I think it makes more sense to use the server/registration as the final part of the package name in the non-api modules

Java packages enforce more restrictions, but make it easier to check we're following our conventions, and over time allow us to clarify the expected visibility of packages

Technically this is only required if we agree to adopt the module system, though split modules are generally not regarded as good practice - and we are extremely good in avoiding them!

@planetf1
Copy link
Member Author

fyi @mandy-chessell @davidradl

@mandy-chessell
Copy link
Contributor

mandy-chessell commented Dec 14, 2022

The split module analysis does highlight some problems that I had been avoiding fixing.

However, the name changes for ffdc above break the naming conventions for the packages. The api package contains the Java interface definitions - it is not the java classes that happen to be in the api module.

If ffdc is in both the api and server modules then I suggest moving the audit log definitions from the server module to the api module. This is consistent with many other modules and is a necessary change when we start generating message manuals.

The naming of the packages around the component definitions in admin are confusing and could do with improving. This is worth doing anyway. I think the appropriate packages should be renamed rather than creating a deeper nested structure.

No concerns about the changes to rest connectors.

On the view services, the packages are typically .../server/spring rather than ../spring.

@mandy-chessell
Copy link
Contributor

I am happy to make the changes in main if that is helpful - or it is going to cause problems with the experiment?

@planetf1
Copy link
Member Author

planetf1 commented Dec 14, 2022

@mandy-chessell The experiment went a bit too far with renaming ;-) I need to untangle that. I wanted to understand the scope first.They were never meant to be the final proposal.

Happy to go with names that make the most sense to you. The key issue we have is 'split packages' ie where the same package name is represented in multiple jars. There's actually very few, but it is completely disallowed for java modules. No warnings etc. it's just a no. (there are magic workarounds which manipulate the jars/classpath .. but best to keep those to third party libraries one can't control)

Suggestions - one of

  • Make the changes in v3 - if you are happy there are no compat concerns
  • Make the changes in v4 - if you think there are compat concerns (but merge harder, the longer we work in parallel)

Then I can remove my class refactoring, & pull in your update & revalidate on my PR -- I'm keeping the module support another level apart for now until we've had a chance to discuss, plus not completed sorting the test cases yet.

Most of the changes for modules are in the creating of the module-info.java & gradle dependencies (to a lesser extent)

If there are changes, splitting them up as much as possible helps with rebase/merge/review. Obviously any change will hit a lot of files, but one rename per commit would be really useful :-) Or one batch of self contained moves (keeping things working).

Currently the module support PR has > 800 file changes :-( I thought it would be pretty quick but it was a bit nasty in places.

Alternatively if you spec out the renames I can do.

mandy-chessell added a commit to mandy-chessell/egeria that referenced this issue Jan 4, 2023
@mandy-chessell mandy-chessell self-assigned this Jan 4, 2023
mandy-chessell added a commit that referenced this issue Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request triage New bug/issue which needs checking & assigning
Projects
None yet
Development

No branches or pull requests

2 participants