Skip to content

feat: matrix generation CLI #2059

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Conversation

HendrikLeuschner
Copy link
Collaborator

@HendrikLeuschner HendrikLeuschner commented May 11, 2025

Pull Request Checklist

  • 1. I have rebased the latest version of the main branch into my feature branch and all conflicts
    have been resolved.
  • 2. I have added information about the change/addition to functionality to the CHANGELOG.md file under the
    [Unreleased] heading.
  • 3. I have documented my code using JDocs tags.
  • 4. I have removed unnecessary commented out code, imports and System.out.println statements.
  • 5. I have written JUnit tests for any new methods/classes and ensured that they pass.
  • 6. I have created API tests for any new functionality exposed to the API.
  • 7. If changes/additions are made to the ors-config.json file, I have added these to the ors config documentation
    along with a short description of what it is for, and documented this in the Pull Request (below).
  • 8. I have built graphs with my code of the Heidelberg.osm.gz file and run the api-tests with all test passing
  • 9. I have referenced the Issue Number in the Pull Request (if the changes were from an issue).
  • 10. For new features or changes involving building of graphs, I have tested on a larger dataset
    (at least Germany), and the graphs build without problems (i.e. no out-of-memory errors).
  • 11. For new features or changes involving the graphbuilding process (i.e. changing encoders, updating the
    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.
  • 12. I have written in the Pull Request information about the changes made including their intended usage
    and why the change was needed.
  • 13. For changes touching the API documentation, I have tested that the API playground renders correctly.

Fixes # .

Information about the changes

  • Key functionality added:Added the CLI interface for the matrix generation. Adapted the http client to reuse connections to avoid overloading the available ports
  • Reason for change: Got errors when generating many matrices.

Examples and reasons for differences between live ORS routes, and those generated from this pull request

Required changes to ors config (if applicable)

@HendrikLeuschner HendrikLeuschner changed the title Feat/matrix benchmark feat: matrix generation CLI May 11, 2025
Copy link
Member

@MichaelsJP MichaelsJP left a comment

Choose a reason for hiding this comment

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

Some minor things.

@aoles aoles requested a review from Copilot May 12, 2025 10:58
@github-actions github-actions bot added feature and removed feature labels May 12, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a new CLI interface for generating matrices for API testing, refactors the HTTP client connection management to reuse connections, and introduces extensive tests for command-line argument parsing and generator functionality.

  • Introduces a new MatrixCommandLineParser for handling CLI arguments.
  • Refactors CoordinateGeneratorMatrix to reuse a shared HTTP client and implements AutoCloseable for proper resource management.
  • Adds thorough tests for CLI parsing and matrix generation scenarios.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
ors-benchmark/src/test/java/org/heigit/ors/coordinates_generator/cli/MatrixCommandLineParserTest.java Adds tests covering valid/invalid argument cases and CLI behavior.
ors-benchmark/src/main/java/org/heigit/ors/coordinates_generator/generators/CoordinateGeneratorMatrix.java Modifies HTTP client handling for connection reuse and resource closure; updates JavaDoc comments.
ors-benchmark/src/main/java/org/heigit/ors/coordinates_generator/cli/MatrixCommandLineParser.java Implements CLI parsing for matrix generation using Apache Commons CLI.
ors-benchmark/src/main/java/org/heigit/ors/coordinates_generator/MatrixGeneratorApp.java Introduces a command-line application entry point for matrix generation.
ors-benchmark/src/main/java/org/heigit/ors/benchmark/MatrixAlgorithmLoadTest.java Provides a load test scenario for matrix API endpoints.
Comments suppressed due to low confidence (1)

ors-benchmark/src/main/java/org/heigit/ors/coordinates_generator/generators/CoordinateGeneratorMatrix.java:352

  • [nitpick] There is a duplicate word 'in' in the JavaDoc comment; please remove the extra 'in' to improve clarity.
 * Routeability in in forward direction between row elements, and column

Comment on lines +232 to +238

// Close HTTP client
try {
httpClient.close();
} catch (IOException e) {
LOGGER.error("Error closing HTTP client", e);
}
Copy link
Preview

Copilot AI May 12, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider consolidating the HTTP client closure logic by centralizing it in the AutoCloseable close() method rather than closing it both in shutdownExecutor and in close().

Suggested change
// Close HTTP client
try {
httpClient.close();
} catch (IOException e) {
LOGGER.error("Error closing HTTP client", e);
}

Copilot uses AI. Check for mistakes.

Copy link
Member

@aoles aoles left a comment

Choose a reason for hiding this comment

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

Nice! 👍

Please consider addressing the issue reported by SonarQube and copilot's improvement suggestions including ones in "Comments suppressed due to low confidence".

Cheers!

@HendrikLeuschner
Copy link
Collaborator Author

Thanks, I adressed all comments, including sonar and copilot.

Copy link
Member

@MichaelsJP MichaelsJP left a comment

Choose a reason for hiding this comment

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

LGTM

@MichaelsJP MichaelsJP requested a review from aoles May 20, 2025 11:53
@MichaelsJP MichaelsJP enabled auto-merge May 20, 2025 11:54
@github-actions github-actions bot added feature and removed feature labels May 20, 2025
@MichaelsJP MichaelsJP removed the request for review from aoles May 20, 2025 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants