Einav Cohen has posted comments on this change. Change subject: restapi: default version to 'general' ......................................................................
Patch Set 3: > i was not talking about of default constructor, but about > GetConfigurationValueParameters(ConfigurationValues cVal), > i.e without the version ... You said in your previous comment: "leave only one constructor"; it doesn't matter which c'tor you meant - we must have also the default c'tor, which doesn't accept any parameters. the only way to solve this *correctly* via c'tors, is for *all* c'tors to accept a (non-null) version parameter, which will enforce the user to define a version. Again, this is impossible since we must have a default c'tor (i.e. a c'tor that doesn't accept any parameters) due to serialization considerations. > my only problem is that we still don't have any tool > to prevent this from happen and as such i think that setting > defaultConfigurationVersion in GetConfigurationValueParameters.privateVersion > will do much better job (for now till we have validation) to reach > the goal of this patch, don't you think? The goal of this patch is not to: 1) eliminate the WARN messages just for the sake of eliminating them. 2) make sure that the backend receives a non-null version. The goal of this patch is to: 3) make sure that the client sends a non-null version (it is different from 2), which is the problem that the WARN message is pointing is pointing on. Your suggestion (setting defaultConfigurationVersion in GetConfigurationValueParameters.privateVersion) will only cover up the problem, as it will solve only 1 and 2; we will potentially be able to add another bunch of "illegal" client calls (i.e. client GetConfigurationValue calls that don't provide Version) and no one will even notice them, since the WARN messages will not be written - this is *much* worse. [need to understand that 2 is not the problem; the backend can, obviously, handle "null" version; the problem is with the client that is not explicitly providing a version when calling to the GetConfigurationQuery, which is wrong by definition; the client *must* know what he is doing, i.e. it needs to request a configuration value for a concrete version; we cannot have the GetConfigurationValue parameters class take care of that for him - it is simply wrong]. I agree that the comprehensive solution should include validation, but again - once we will introduce it, any "illegal" call which exists in the clients (i.e. without passing the version parameter) will fail, which will result in bugs/regressions. so need to fix these "illegal" calls first, which is what this patch is all about. we are not doing any unnecessary work here - we will have to fix these calls sooner or later. If we have already found these problematic calls and sent a patch to fix them - why not merge it? Having said that, I see your point of potentially adding to the code new "illegal" calls in the mean time (until the comprehensive solution will be introduced). so if you completely object to doing the comprehensive solution in a separate patch and you want everything in a single patch - than we will abandon this patch (or update it with the comprehensive solution once we will have the time to do it). -- To view, visit http://gerrit.ovirt.org/11173 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I946c9925f3e0b94a9dad86f0958c2f9e1d0ab5c8 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Alissa Bonas <abo...@redhat.com> Gerrit-Reviewer: Einav Cohen <eco...@redhat.com> Gerrit-Reviewer: Michael Pasternak <mpast...@redhat.com> Gerrit-Reviewer: Ori Liel <ol...@redhat.com> _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
