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

[GEOT-7422] Adapt to GeoTools upgrade of java units "indriya" to version 2.2 #7545

Merged
merged 1 commit into from
Jun 3, 2024

Conversation

soc
Copy link
Contributor

@soc soc commented Apr 12, 2024

GEOT-7422 Powered by Pull Request Badge

Companion change to geotools/geotools#4690.

Checklist

For core and extension modules:

  • New unit tests have been added covering the changes.
  • Documentation has been updated (if change is visible to end users).
  • The REST API docs have been updated (when changing configuration objects or the REST controllers).
  • There is an issue in the GeoServer Jira (except for changes that do not affect administrators or end users in any way).
  • Commit message(s) must be in the form [GEOS-XYZWV] Title of the Jira ticket.
  • Bug fixes and small new features are presented as a single commit.
  • Each commit has a single objective (if there are multiple commits, each has a separate JIRA ticket describing its goal).


package org.geoserver.netcdf;

import org.geotools.imageio.netcdf.NetCDFUnitFormat;
Copy link
Member

@aaime aaime Apr 15, 2024

Choose a reason for hiding this comment

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

This is not going to work. The units configuration is there to support reads too (see VariableAdapter usage in GeoTools), otherwise the class would be placed in netcdf-out.

However, I don't see why you can't use NetCDFUnitFormat directly, rather than creating NetCDFUnitFormatInstance (all it does is to create a second instance of the format, that is then being re-configured).

If mutability is the issue, thinking out loud, we could have a way to build a new NetCDFUnitFormat internal instance, and just throw away the old one: this way the unit format would be immutable, for as long as it lives. Just inform the callers not to cache the formatter instance, but always get it fresh from NetCDFUnitFormat.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! You mean that we also use that instance from VariableAdapter then?

Copy link
Member

Choose a reason for hiding this comment

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

I mean the configuration file in GeoServer should affect also VariableAdapter parsing. In other words, the configuration is used also to parse NetCDF files with uncommon units, and not only to control the generation of new NetCDFs in the GeoServer output format.

@aaime
Copy link
Member

aaime commented Apr 15, 2024

Another thing I've noticed, gs-main won't build unless I control the unit-api dependency in the main GeoServer pom file, adding it to the dependency management.
Otherwise, the transitive dependency from systems.uom takes over and brings me back to an older version of javax.units, that does not have the QUETTA constant.

@soc
Copy link
Contributor Author

soc commented Apr 15, 2024

Thanks, will try addressing these things!

…d NetCDFUnitFormat instance instead of global state
@soc
Copy link
Contributor Author

soc commented Apr 28, 2024

Reverted the undesired changes, added the unit-api dependency.

@aaime aaime changed the title [GEOT-7422] adapt to GeoTools changes, operate on GeoServer-controlled NetCDFUnitFormat instance instead of global state [GEOT-7422] Adapt to GeoTools changes May 3, 2024
@jodygarnett jodygarnett changed the title [GEOT-7422] Adapt to GeoTools changes [GEOT-7422] Adapt to GeoTools upgrade of java units "indriya" to version 2.2 May 17, 2024
@@ -40,7 +40,7 @@ private void configure() {
LinkedHashMap<String, String> aliases =
getMapResource(NETCDF_UNIT_ALIASES, NetCDFUnitFormat.NETCDF_UNIT_ALIASES);
if (aliases != null) {
NetCDFUnitFormat.setAliases(aliases);
NetCDFUnitFormat.getInstance().setAliases(aliases);
Copy link
Member

Choose a reason for hiding this comment

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

The changes look good but setAliases is private in the GeoTools PR.

Did you forget to push some changes over there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, but I updated the PR to keep them public, can you try again? :-)

@@ -51,7 +51,7 @@ private void configure() {
getMapResource(
NETCDF_UNIT_REPLACEMENTS, NetCDFUnitFormat.NETCDF_UNIT_REPLACEMENTS);
if (replacements != null) {
NetCDFUnitFormat.setReplacements(replacements);
NetCDFUnitFormat.getInstance().setReplacements(replacements);
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, this is also private in the corresponding GeoTools PR.

@aaime
Copy link
Member

aaime commented Jun 3, 2024

Looks good, thanks, merging!

@aaime aaime merged commit c22df3e into geoserver:main Jun 3, 2024
2 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants