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

Fix merging of composite availability #7718

Merged
merged 4 commits into from
Apr 26, 2019
Merged

Fix merging of composite availability #7718

merged 4 commits into from
Apr 26, 2019

Conversation

emackey
Copy link
Contributor

@emackey emackey commented Apr 5, 2019

Fixes #7717

The name and availability weren't being cleaned by the CompositeEntityCollection's clean helper, so re-merging them wasn't taking place by order of composed collections, instead it was whatever the order that the underlying collections were being populated by the server, which was a race condition.

At some point, the merge for availability had intentionally been installed backwards, possibly as a workaround for this race condition in a particular app.

@cesium-concierge
Copy link

Thanks for the pull request @emackey!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@emackey
Copy link
Contributor Author

emackey commented Apr 5, 2019

I need to fix the merge tests, they test for the bad behavior.

@emackey
Copy link
Contributor Author

emackey commented Apr 6, 2019

I added a new test to ensure that availability cleans & merges correctly like the other properties. The new test succeeds here and fails in master (with master showing the lowest-priority collection's availability is the winner in that case).

I also modified one other test in a way I'm less certain of. This test specifically tested for merge to treat availability the opposite way from everything else, and here I've modified the test to treat it as a normal merge, so availability aligns with the other merged properties. I strongly suspect that this intentional backwards-ness was placed there as a workaround for the bug where clean didn't clean the availability.

Ultimately, in master, the chosen availability for any one entity doesn't come from the ordering of collections in the composite at all. Due to the lack of cleaning, it comes from the order in which the collections happened to be first populated in the composite. On this branch, the availability gets cleaned and merged in the same order as all the other entity properties, so it will come from the same source that provided the time-dynamic properties, which seems like a good thing.

It would be great for this to sit in master for a while, to get some testing. I've done my own testing where I intentionally defer loading one collection or the other, and it's clear that master has a race condition there that is fixed here.

@emackey
Copy link
Contributor Author

emackey commented Apr 8, 2019

The Travis fail is unrelated to tests on this branch. This is ready.

@emackey
Copy link
Contributor Author

emackey commented Apr 17, 2019

@mramato Any chance you can take a look at this?

@mramato
Copy link
Contributor

mramato commented Apr 26, 2019

Sorry for the slow roll here @emackey. This looks like a straightforward fix and I'm sure if there was a good reason it was flipped (spoiler alert, probably not) we'll be reminded of this soon enough. Thanks again!

@mramato mramato merged commit fbd39d0 into master Apr 26, 2019
@mramato mramato deleted the merge-availability branch April 26, 2019 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merged Entity availability
3 participants