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

Broken graph after removing subnetworks, then adding nodes #1313

Closed
michaz opened this issue Mar 18, 2018 · 5 comments
Closed

Broken graph after removing subnetworks, then adding nodes #1313

michaz opened this issue Mar 18, 2018 · 5 comments
Assignees
Labels

Comments

@michaz
Copy link
Member

michaz commented Mar 18, 2018

Adding edges (or, at least, adding edges to new nodes) to a graph after having removed disconnected subnetworks results in a corrupted graph:

7f82541

@karussell karussell self-assigned this Mar 18, 2018
@karussell karussell added the bug label Mar 18, 2018
@karussell
Copy link
Member

karussell commented Mar 18, 2018

This is ugly - thanks for finding and already writing the failing test :) 👍 !

We currently remove the nodes in the graph and do the compaction in-place to avoid holding two (potentially really big graphs) in memory. But we do this only for the nodes and leave the invalid edges in the graph.

BTW: Maybe we should do the same for the nodes (leaving them in the graph) and do the compaction only via the simpler graph copying mechanism as this in-place compaction made lot's of problems and changes the node-IDs and potentially does not save so much RAM that the problems are justified. (graph copying is already done if graph sorting is enabled then we call GHUtility.sortDFS(ghStorage, newGraph))

@michaz
Copy link
Member Author

michaz commented Mar 18, 2018

Yes, I must admit I had a moment of panic where I was afraid that we would be changing the edge ids. (I hold on to them and save them and do lookups on them!) But luckily we aren't.

(I don't care about nodes.)

@karussell
Copy link
Member

I think the problem is that we do not reset the pointers to the edges of the invalidated nodes in the graph.optimize() call. So in the test the edge 7-8 is created and 8 is still linking to some old edges and we append now 7-8 to the linked list and reanimate an edge with invalid nodes.

The fix is here: 6be91f3

@karussell
Copy link
Member

Strange, this does not make sense because we call baseGraph.trimToSize();

@karussell
Copy link
Member

karussell commented Mar 18, 2018

Ah, we keep only the necessary segment files but they could still partially filled with old data.

michaz added a commit that referenced this issue Mar 27, 2018
…e the root problem is now fixed. #1311 #1313

This reverts commit 24bf137

Signed-off-by: Michael Zilske <michael.zilske@tu-berlin.de>
karussell added a commit that referenced this issue Apr 4, 2018
karussell pushed a commit that referenced this issue Apr 7, 2018
* Initial attempt at dropwizardifying

* Remove CmdArgsModule

* For example: ChangeGraph

* For example: Nearest

* No more Guice, no more Servlets

* Bundle configuration; mergin of system properties

* config-example.yml

* Add OSM file name to example

* Add port number to example config

* Dropwizard import command

* ChangeGraphResource is disabled by default

* rename hk2 factories to *Factory

* redirect to webapp from root

* dropwizard 1.1.2

* Add banner.txt

* Restore tests for http error status codes, and fix it.

* Insert separate fields for planned/predicted departure/arrival time

Signed-off-by: Michael Zilske <michael.zilske@tu-berlin.de>

* Set the expected times fields only when there is an update.

Signed-off-by: Michael Zilske <michael.zilske@tu-berlin.de>

* Insert separate fields for planned/predicted departure/arrival time

* Bump Dropwizard version; declare J2EE dependencies (for Java 9)

Signed-off-by: Michael Zilske <michael.zilske@tu-berlin.de>

* Use the same Jackson version in the client.

Signed-off-by: Michael Zilske <michael.zilske@tu-berlin.de>

* Remove client-duplicate, as in master

Signed-off-by: Michael Zilske <michael.zilske@tu-berlin.de>

* Adapt to refactoring

Signed-off-by: Michael Zilske <michael.zilske@tu-berlin.de>

* Use shade plug-in as per Dropwizard doc

Signed-off-by: Michael Zilske <michael.zilske@tu-berlin.de>

* Disable Dropwizard admin port in default config

Signed-off-by: Michael Zilske <michael.zilske@tu-berlin.de>

* Remove empty files

Signed-off-by: Michael Zilske <michael.zilske@tu-berlin.de>

* Fix banner

Signed-off-by: Michael Zilske <michael.zilske@tu-berlin.de>

* Move webapp from /webapp to /maps

Signed-off-by: Michael Zilske <michael.zilske@tu-berlin.de>

* Disable request log

Signed-off-by: Michael Zilske <michael.zilske@tu-berlin.de>

* Add a (not very strong) Dropwizard health check to each of the two API variants.

I think this is okay for now -- they are thinking more of testing the health of external services such as databases.

Signed-off-by: Michael Zilske <michael.zilske@tu-berlin.de>

* Adapt graphhopper script for dropwizard branch (#1319)

* adapt graphhopper.sh script for dropwizard

* now variable JETTY_PORT works (empty would result in random so no parameter is required if empty); avoid starting admin connector

* minor fix

* improve security and bind to private localhost instead of public 0.0.0.0, then we can start admin panel and remove IPFilter

* improve script and include some shortcut to specifiy vehicle profiles and the gtfs input file

* remove logging echo statement

* 'detaching' does not need a value

* minor improvements

* forward parameters as there won't be compatibility issues due to the '-' prefix; use -- for parameters longer than 2 characters; added possibility to print version

* minor cleanup

* sort alphabetically

* added author when adapting graphhopper.sh script for dropwizard #1319

* avoid unnecessary dependency like commons logging

* merged master

* use WebApplicationException

* no tools jar possible, use web jar via ImportCommand

* updated docs

* revert change explicitely #1313
michaz added a commit that referenced this issue Apr 22, 2018
…e the root problem is now fixed. #1311 #1313

This reverts commit 24bf137

Signed-off-by: Michael Zilske <michael.zilske@tu-berlin.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants