Consult UUIDs when deciding to destroy vizelements.#3838
Conversation
I suspect that a number of bad visualization behaviors after shard reset are the result of IDs being reclaimed by different clients/entities, causing vizelements to be reassigned instead of destroyed and recreated. Here, we make vizelements consult UUIDs as well so that if a new element shows up with the same entity ID, the client knows it's a different element and will destroy the old element and create a new one. My hope is that this will eliminate a whole class of bugs. I at least know that before this change I was able to recreate some connected router bugs, etc, in local testing, and after this change I was no longer able to do so.
There was a problem hiding this comment.
Could we do either (ID, UUID) or (Id, Uuid)? Mixing feels really weird.
There was a problem hiding this comment.
Sure. I think I got this habit from the old MSDN rules for capitalization of acronyms wherein two-character acronyms could be all capitalized but 3+ character acronyms only capitalize the leading letter.
Re-reading those rules, ID is an abbreviation, not an acronym, and should be written id or Id unlike two-letter acronyms. I'll go with Id / Uuid. Maybe I can do a pass to normalize this throughout NetSim soon.
There was a problem hiding this comment.
That ... is a pretty cool resource. Sounds good to me!
|
I'd love to see some tests |
Test to show that the visualization will destroy old entities and generate new ones when the shard resets, even if the new entities have the same IDs as old ones. [ci skip]
|
@Hamms PTAL. That was kind of a tricky test to write. |
There was a problem hiding this comment.
what kind of entities do not have IDs? seems like a reasonable check though.
There was a problem hiding this comment.
The inheritance chain for stuff in the visualization looks something like this:
NetSimVizElement
NetSimVizNode
NetSimVizAutoDnsNode
NetSimVizSimulationNode
NetSimVizWire
NetSimVizSimulationWire
Only the ones with Simulation in the name actually map to simulation entities - they are the only ones that implement getCorrespondingEntityID. This is a place where I'd like to use an interface, but duck typing will have to suffice.
Things are inherited this way because NetSimVizSimulationNode actually has a whole lot more in common with NetSimVizAutoDnsNode than it does with NetSimVizSimulationWire. There might be a good composition solution to this problem though.
There was a problem hiding this comment.
This comment is confusing because it seems like "simulation node" might refer to a NetSimVizSimulationNode, whereas it actually refers to a NetSimNode. It might be easier to read if you just used type names in comments, e.g. by changing "simulation node" to NetSimNode (here and above).
|
LGTM. I think I follow what's going on well enough. I've noted inline where comments could be changed to make it easier for someone like me with non-expert levels of netsim knowledge to understand what's going on. |
|
Thanks Dave! That's some great feedback on comments around this feature. I'll update the write-up and then merge this in. |
Consult UUIDs when deciding to destroy vizelements.
commit fe06578 Merge: 4e0de33 fe7e738 Author: Brad Buchanan <bradley.c.buchanan@gmail.com> Date: Fri Sep 4 13:02:13 2015 -0700 Merge pull request #3838 from code-dot-org/netsim-visualization-reset Consult UUIDs when deciding to destroy vizelements. commit fe7e738 Author: Brad Buchanan <brad@code.org> Date: Fri Sep 4 12:38:39 2015 -0700 Better comments. commit 4e0de33 Merge: 24ae4ae 1c955c3 Author: Will Jordan <wjordan@users.noreply.github.com> Date: Fri Sep 4 12:25:55 2015 -0700 Merge pull request #3877 from code-dot-org/rails_4_2_update Rails 4 2 update (extra fixes #2) commit 1c955c3 Author: Will Jordan <will@code.org> Date: Fri Sep 4 12:21:28 2015 -0700 load i18n locales before application fork commit 24ae4ae Merge: 8a999a3 6f3e0a7 Author: Andre Stackhouse <CaptainStack@outlook.com> Date: Fri Sep 4 12:12:26 2015 -0700 Merge pull request #3876 from code-dot-org/automated-workshop-email-reminder-prerequisites Much simpler prerequisite calculation (not using activity_constants)
I suspect that a number of bad visualization behaviors after shard reset are the result of IDs being reclaimed by different clients/entities, causing vizelements to be reassigned instead of destroyed and recreated.
Here, we make vizelements consult UUIDs as well so that if a new element shows up with the same entity ID, the client knows it's a different element and will destroy the old element and create a new one. My hope is that this will eliminate a whole class of bugs. I at least know that before this change I was able to recreate some connected router bugs, etc, in local testing, and after this change I was no longer able to do so.
Continuing work on Pivotal #101529076