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

Update Java dependencies #903

Merged
merged 34 commits into from
Oct 10, 2019

Conversation

adriangonz
Copy link
Contributor

@adriangonz adriangonz commented Oct 2, 2019

This PR addresses issue #902.

Changelog

We will split the changelog by the update increments shown in issue #902. Changes are only shown for version updates which required changes on the source code other than just a version change in pom.xml.

Update spring and spring-boot

Update to [email protected]

  • Add spring-boot-properties-migrator to help migrating application properties. This is recommended on the upgrade guide.
  • Something on the internal structure of RestTemplate has changed. As a consequence, on the tests, we now need to mock the restTemplate.getRequestFactory() method as well.
  • Changes related to embedded web servers and containers:
    • All the org.springframework.boot.context.embedded imports now point to org.springframework.boot.web.servlet and org.springframework.boot.web.embedded. This is more detailed in this section of the upgrade guide.
    • The *Customizer classes have also changed slightly in its structure. More info can be found here.
  • micrometer-spring-legacy is no longer required, since it's already included into [email protected]. All the io.micrometer.spring imports now point to org.springframework.boot.actuate.metrics.
  • Changes to spring-boot-actuator endpoints:
    • The configuration of actuator endpoints has changed. There is no longer a endpoints.prometheus.sensitive attribute, since that's now handled explicitly by spring-security. Since we don't use spring-security, we can remove that line. Likewise, there is no longer a endpoints.prometheus.id attribute, since the id is now considered immutable.
    • To add back the previous /prometheus endpoint, we enabled it by adding it to the management.endpoints.web.exposure.include list and changed the base path of the actuator endpoints to / with management.endpoints.web.base-path.
    • Besides /health, /info and /prometheus, every other spring-boot-actuator endpoint will now return a 404. Before, this used to be a 401. To bring back the 401s, we would need to introduce spring-security, enable these endpoints and configure the 401 there.
  • It seems that before, @Scheduled tasks were enabled indirectly by some other piece of config. However, after the update, we are required to enable them explicitly adding the @EnableScheduling annotation. This affects the SeldonGraphReadyChecker, since it checks periodically if the inference graph is ready. https://github.com/SeldonIO/seldon-core-mirror1/blob/ca4ae76a9379cdb47eaa1ecc211eb43e34f874dd/engine/src/main/java/io/seldon/engine/api/rest/SeldonGraphReadyChecker.java#L111-L118

Update com.google.protobuf packages

  • Introduce ${grpc.version} variable to pom.xml and set to v3.9.2.

Update opentracing-grpc, opentracing-rest and opentracing-api packages

Update org.apache.curator packages

  • Introduce ${curator.version} variable to pom.xml and set to v4.2.0.
  • Replace internal org.jboss.netty.util.internal.ConcurrentHashMap for java.util.concurrent.ConcurrentHashMap.

@axsaucedo
Copy link
Contributor

Looks good IMO, all changes seem to be non-breaking, and major ones are only on tracing. Would be good to get signoff from @cliveseldon

@axsaucedo
Copy link
Contributor

/uncc @arnaudvl

@seldondev seldondev removed the request for review from arnaudvl October 3, 2019 18:52
@axsaucedo
Copy link
Contributor

/cc @cliveseldon

@seldondev seldondev requested a review from ukclivecox October 3, 2019 18:52
@axsaucedo
Copy link
Contributor

@adriangonz is this PR still WIP?

@adriangonz
Copy link
Contributor Author

Thanks for your feedback @axsaucedo ! I still haven’t got through the full set of dependencies, that’s why I consider it as WIP. However, to keep it small, perhaps it would make sense to split it into smaller chunks? I’m confident about all the changes prior to the tracing updates.

Sent with GitHawk

@ukclivecox
Copy link
Contributor

@adriangonz Just a few questions. Apart from that seem good. Might want to run the new e2e tests in Kind to check they all pass and then we can approve.

@adriangonz adriangonz changed the title WIP : Update Java dependencies Update Java dependencies Oct 8, 2019
@adriangonz
Copy link
Contributor Author

@axsaucedo @cliveseldon I just finished updating all the first-level Java dependencies. All the relevant changes are listed on this PR and you can find a brief discussion on changes for each package in #902.

@ukclivecox
Copy link
Contributor

/approve

@seldondev
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cliveseldon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@seldondev seldondev merged commit ec18ddf into SeldonIO:master Oct 10, 2019
@adriangonz adriangonz deleted the update-java-dependencies branch October 10, 2019 14:12
@seldondev
Copy link
Collaborator

failed to trigger Pull Request pipeline

  • failed to create agent
  • failed to calculate in repo config
  • failed to load trigger config for repository SeldonIO/seldon-core for ref 27d9cf4
  • failed to find any lighthouse configuration files in repo SeldonIO/seldon-core at sha 27d9cf4
  • failed to process repo SeldonIO/seldon-core refref 27d9cf4
  • failed to list files in directory /var/tmp/gitrepo660600678/.lighthouse
  • open /var/tmp/gitrepo660600678/.lighthouse
  • no such file or directory

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants