Juan Hernandez has posted comments on this change. Change subject: restapi: optimize getUriBuilder ......................................................................
Patch Set 2: (1 comment) http://gerrit.ovirt.org/#/c/37988/2/backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/utils/LinkHelper.java File backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/utils/LinkHelper.java: Line 813: Collection collection = getCollection(model, suggestedParentType); Line 814: if (collection == null) { Line 815: return null; Line 816: } Line 817: UriBuilder uriBuilder = builderMap.get(collection); Using the collection as the key of the map improves the situation. But I still have two concerns: 1. The "Collection" class doesn't implement the "equals" and "hashCode" methods, so this will only work correctly if we trust that all the instances of "Collection" that we create are actually different. That is probably a safe assumption at the moment, because of how we create them. Anyhow, I'd suggest to implement the "equals" and "hashCode" methods correctly, just in case. 2. The state of class UriBuilderImpl (the default implementation of UriBulider) includes things like the identifiers of entities. For example, when calling this method to build the link for a VM it will store the the following path in its "path" field: /ovirt-engine/api/vms/84f3a784-d78f-468b-88d0-cbe8133e6a77 What will happen when building the next VM? Nothing very bad, because we will take the builder from the cache with the old data and then remove the identifier and replace it with the new one. But still, this is kind of leaking information from one request to a different one, seems potentially dangerous to me. Can we try to modify this so that the methods that potentially populate the cache never receive the model instance? I mean, if the method receives only "Class modelClass" instead of "R model" then there is no chance it can get information specific to a particular entity and put it in the cache. Line 818: Line 819: if (uriBuilder == null) { Line 820: Line 821: if (collection.getParentType() != NO_PARENT) { -- To view, visit http://gerrit.ovirt.org/37988 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I86221e8af24da28a7137c0cc0e7aec69943a65c5 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Einav Cohen <eco...@redhat.com> Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
