feat: config migration and graph repo client #1889
Conversation
…ure into ors-engine - The config is now entirely encapsulated in a clear class hierarchy that can be imported by any module needed. Ors-api imports those classes and is able to use them directly with the externalized spring-boot config loader functionality. - The config classes are using jackson to deserialize yaml files. Since its the basic jackson logic, theoretically ors should understand all possible file formats jackson can handle (yml,json etc.). But we assume yml only. - All formerly bare string accesses to config parameters are now clear function and method accesses from the config classes. - The config class engine has a new parameter graphManagement for a new graph repo client which will be added in subsequent commits - The config classes are currently not designed for serialization except the graph management configs. - Some obvious generic getter/setter scenarios have now been implemented or replaced with the corresponding lombok imports. - ProfileProperties and ProfileDefaultProperties have been merged. ProfileProperties has new parameters `build`, `repo`, and `service`. Except repo the other two include no new parameters, just moved ones. Repo is new and is part of the coming graph management feature. Co-authored-by: jhaeu <[email protected]> Co-authored-by: Julian Psotta <[email protected]> BREAKING CHANGE: Structural changes in the configuration make this change incompatible with earlier versions and configurations.
…pository - The feature allows ors to download graphs from a pre-defined remote repository. The ors client looks for specific things like graph version, the used profiles, area of interest etc. to decide which graph should be downloaded. - To differentiate incompatible graphs, these changes are accommodated by a manually defined graph-version. Once a version is different, the graphs are incompatible. - While building new graphs, the folder of the graphs will include YAML files that contain the settings the graphs were built with. This ensures that graphs cannot be loaded with different graph building relevant configs. - Downloaded graphs will always contain these files. - Graph download triggers can be configure by schedule as well as its activation. Co-authored-by: Takara Baumbach <[email protected]> Co-authored-by: Julian Psotta <[email protected]>
… with tests based on junit and TestContainers The focus of these tests is not to check for correct API tests but for the correct interaction with external tools and dependencies: - Config environment tests - Config file tests - Config lookup tests - GeoTools temp folder creation test - GraphRepoTests that mock an external graph repo with different scenarios The new junit based integration tests are executed like any other junit tests. Under the hood they use test containers to spawn containers with docker to provide isolated and clear defined environments. All tests run in the same way for all current relevant execution scenarios, maven/spring-boot, war/tomcat, java/jar and test for the same expected behavior. All tests are executed in parallel. The max parallel tests are configured by `junit-platform.properties`. Before each parallel execution the container layers are built with a single thread to ensure that parallel tests reuse the same image layers for the containers. This leads to a massively reduced build time in general. Graphs are pre-built by the same layer caching process before the individual test run and shared among tests that request shared graphs. Some tests require new graphs on the fly. Co-authored-by: jhaeu <[email protected]> Co-authored-by: Takara Baumbach <[email protected]>
The documentation now includes information for the new configuration of ors. The configuration is now differently structured and thus content moved a lot. Additionally, the graph download functionality is described. Co-authored-by: Takara Baumbach <[email protected]>
MichaelsJP
changed the title
the latter is flaky because the format depends on local locale, time zone etc.
|
The commit message title of commit 6035301 is irritating. A refactoring is not supposed to be a breaking change. |
The ! indicates a breaking change & the body describes the thing that is breaking: according to the specification we are currently using, that's how a breaking change should be committed and it can be on any commit type: Or do you think the actual title confusing? |
|
No, I think that in general "refactor" and "BREAKING CHANGE" are incompatible by their definition. I wonder whether there are rare exceptions to this? |
|
ah due to refactoring being defined by "not altering existing functionality" 😓. Yeah, agreed. |
| @@ -40,7 +40,7 @@ | |||
| #ors.endpoints.matrix.maximum_routes_flexible=25 | |||
| #ors.endpoints.matrix.maximum_visited_nodes=100000 | |||
| #ors.endpoints.matrix.maximum_search_radius=2000 | |||
| #ors.endpoints.matrix.u_turn_costs=-1 | |||
| #ors.endpoints.matrix.u_turn_cost=-1 | |||
There was a problem hiding this comment.
Why has this changed? We use the plural form of "costs" everywhere else.
| prepareGreenIndexSlots(); | ||
| ExtendedStorageProperties parameters; | ||
| try { | ||
| parameters = this.parameters; |
There was a problem hiding this comment.
How is this assignment supposed to fail? The same question applies to the other GraphStoragBuilders.
There was a problem hiding this comment.
Why has the format been changed? I think XML was on purpose as the file is manually adapted to account for special test cases. XML would make it easier to inspect changes or add new changes for new tests.
|
|
||
| @Configuration | ||
| @ConfigurationProperties(prefix = "ors.engine") | ||
| public class EngineProperties extends org.heigit.ors.config.EngineProperties { |
There was a problem hiding this comment.
I don't like that the child class has the same name as the parent class.
| @@ -24,6 +26,9 @@ | |||
| import java.util.*; | |||
|
|
|||
| public class IsochroneRequest extends ServiceRequest { | |||
| @Getter | |||
| @Setter | |||
| private String profileName; | |||
There was a problem hiding this comment.
Lombok should not be used here because:
- It reduces only a single getter/setter
- The usage of this field is semantically not a getter/setter
| @@ -40,6 +42,9 @@ | |||
| import org.locationtech.jts.geom.Coordinate; | |||
|
|
|||
| public class MatrixRequest extends ServiceRequest { | |||
| @Getter | |||
| @Setter | |||
| private String profileName; | |||
There was a problem hiding this comment.
Lombok should not be used here because:
- It reduces only a single getter/setter
- The usage of this field is semantically not a getter/setter
| @@ -31,55 +32,38 @@ | |||
|
|
|||
| public class GraphProcessContext { | |||
| private static final Logger LOGGER = Logger.getLogger(GraphProcessContext.class.getName()); | |||
| private List<GraphBuilder> graphBuilders; | |||
| private GraphBuilder[] arrGraphBuilders; | |||
| @Getter | |||
There was a problem hiding this comment.
Lombok should not be used here because:
- It reduces only a single getter/setter
- The usage of this field is semantically not a getter/setter
| private List<GraphStorageBuilder> storageBuilders; | ||
| private GraphStorageBuilder[] arrStorageBuilders; | ||
| private int trafficArrStorageBuilderLocation = -1; | ||
| @Getter | ||
| private final double maximumSpeedLowerBound; |
There was a problem hiding this comment.
Lombok should not be used here because:
- It reduces only a single getter/setter
- The usage of this field is semantically not a getter/setter
| import org.locationtech.jts.geom.Coordinate; | ||
|
|
||
| public class SnappingRequest extends ServiceRequest { | ||
| @Getter | ||
| @Setter | ||
| private String profileName; |
There was a problem hiding this comment.
Lombok should not be used here because:
- It reduces only a single getter/setter
- The usage of this field is semantically not a getter/setter
| @Scheduled(cron = "${ors.engine.graph_management.download_schedule:0 0 0 31 2 *}")//Default is "never" | ||
| public void checkForUpdatesInRepo() { | ||
|
|
||
| if (!enabled) { |
| } | ||
|
|
||
| public void checkForDownloadedGraphsToActivate(String trigger) { | ||
| if (!enabled) { |
| @Async | ||
| @Scheduled(fixedDelay = 1, timeUnit = TimeUnit.MINUTES) | ||
| public void repeatedGraphActivationAttempt() { | ||
| if (!enabled) { |
| if (!enabled) { | ||
| LOGGER.debug("Graph management is disabled, skipping scheduled repository check..."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
We repeatedly have to check for being enabled. Shouldn't this be handled through polymorphism?
There was a problem hiding this comment.
What makes this class and its child classes RepoManagers instead of Repos?
There was a problem hiding this comment.
GraphInfo is a very generic name. Can we provide a better one that expresses the content of this class and its subclasses?
Split the configuration documentation into two logical subpages. Co-authored-by: Julian Psotta <[email protected]>
This commit truly isolates the integration tests scenarios. Before testcontainers would always randomly cache unrelated parts of the dockerfiles. With this setup it doesn't happen anymore and the CI workflow will be able to run way faster.
The new workflow first builds and caches the builder image. This image is big and clunky and would only increase the parallel test time massively. The testing graph is build in the same way. The image ID from the image is used to check compatibility with the graph. When all that works the integration tests are started in parallel for all scenarios and test classes.
The tests are too heavy to be run all the time. Let's limit them to PR and push on main
The integration tests are quite powerful but also more complex than usual tests. Hence, they need some deeper explanation.
This way all workflows now use the same caches.
…yml and ors-config.env
This should have been done way earlier already.
Before it was still run with the other "test" maven stage classes. That is not desired as it would spin a
|
Pull Request Checklist
have been resolved.
[Unreleased] heading.
along with a short description of what it is for, and documented this in the Pull Request (below).
(at least Germany), and the graphs build without problems (i.e. no out-of-memory errors).
importer etc.), I have generated longer distance routes for the affected profiles with different options
(avoid features, max weight etc.) and compared these with the routes of the same parameters and start/end
points generated from the current live ORS.
If there are differences then the reasoning for these MUST be documented in the pull request.
and why the change was needed.
Fixes # .
Information about the changes
Examples and reasons for differences between live ORS routes, and those generated from this pull request
Required changes to ors config (if applicable)