I am forwarding this thread between me, Aaron, and Dan on changes to the Quantum API (filters and pagination). This should have been on the gerrit review page, but that system is not really helpful when discussing alternatives for a patch.
I am forwarding this thread mainly to give an heads up concerning the fact that with this change we are going to change two methods in the plugin interface, get_all_networks and get_all_ports. In particular we are proposing to: 1) add a **kwargs parameter to both of them. This parameter will receive filtering and pagination options. 2) Modify the return type, which currently is a list of networks [ports] to a pair constituted by a list of networks [ports], and a dictionary of non-consumed filtering options, which basically are the options the plugin did not know how to handle. During today's meeting we agreed (Dan please correct me if I'm wrong) that we will try and finalize filters (https://review.openstack.org/#change,2968) for E-3, while we will defer pagination (https://review.openstack.org/#change,3141) to E-4. Thanks, Salvatore From: Dan Wendlandt [mailto:d...@nicira.com] Sent: 24 January 2012 17:26 To: Salvatore Orlando Cc: Aaron Lee Subject: Re: Change in openstack/quantum[master]: bug/801227 API should support paginated collections From: Aaron Lee [mailto:aaron....@rackspace.com<mailto:aaron....@rackspace.com>] Sent: 24 January 2012 16:23 To: Salvatore Orlando Cc: Dan Wendlandt Subject: Re: Change in openstack/quantum[master]: bug/801227 API should support paginated collections I just talked over another idea with Vek and jkoelker. If we require the plugin to return the unused filter_opts. This would leave the calls from the API looking like: (networks, unused_filter_opts) self._plugin.get_networks(tenant_id, filter_opts=filter_opts) filtered_networks = filters.filter_networks(networks, self._plugin, tenant_id, unused_filter_opts) I think this is the easiest way to handle communication back to the API, any filter args that are returned from the query can be used by filter_networks. If filter_networks receives no unused_filter_opts it does nothing. This will also allow us to add query params that do not have to be handled by the plugins. As long as they don't remove them from the query they will get passed onto the filter_networks. I think this gives a fairly clean "hybrid" inside the plugin and outside the plugin approach. This is actually one of the ways I was considering for translating into code what Dan proposed about plugins supporting only a subset of filters. And yes, it will allow us to remove the plugin_can_filter methods, and any conditional statement in the api. This makes sense to me. This methodology could also be applied to the pagination params. Pass pagination_params into get_networks and have it return unused_pagination_params. Returning a threeple is a bit much, maybe that could be a dict? The parameters for pagination are fixed ('limit' for the number of elements and 'marker' for the starter element), and optional as well (without 'marker' response starts from first element, default 'limit' is specified in configuration file), I think we will better off by stating that the plugin interface must return a triple (networks, unused_filter_opts, paginated), where - Networks is an iterable containing network resources - Unused filter ops is a dictionary with the filtering options not consumed by the plugin - Paginated is a Boolean Alternatively, we can treat pagination as any other filtering options, and add the pagination feature to the filtering routine. The 'paginator' would therefore be just like any other filter. In this case, the plugin will return just a couple. or as we in the use would say, a pair :) Do you think it is best to change the return value, or pass in the filtering ops dictionary by reference using kwargs? The latter strikes me as offering a bit more flexibility with respect to a plugin that is unaware, b/c as long as its signature accepts kwargs, it will simply ignore the kwargs that it knows nothing about. However, a plugin unaware of a change in expected return value will cause errors. Not sure about this though. This apparently sounds quite clever, although the fact that a filter does pagination might sound a bit stinky. Yeah, I was actually wondering myself whether this model is reasonable. Pagination is not quite filtering though, as it is not composable. Instead, pagination must be performed after all filters have been applied. We could override the filters dictionary to carry both bits of info though. We also still need to understand whether it is ok to alter the plugin interface at this stage in the release cycle. Doing so later E-4 would not be ideal, but at this point I think we're still figuring out the plugin interface, so I think a change in E-3 early E-4 would be reasonable. Particularly if the change is just requiring plugins to accept kwargs if they can choose to ignore them without consequence. Dan Aaron On Jan 24, 2012, at 3:50 AM, Salvatore Orlando wrote: I think there's no argument about doing any hybrid operation here. Things like finding the marker in the plugin and then fetching a given number of items in the API layers just don't make a lot of sense to me. However, it is true that there's a chance any decision we might take on bp/api-filters is likely to impact this changeset as well. Salvatore From: Dan Wendlandt [mailto:d...@nicira.com<mailto:d...@nicira.com>] Sent: 24 January 2012 09:47 To: Salvatore Orlando Cc: Aaron Lee Subject: Re: Change in openstack/quantum[master]: bug/801227 API should support paginated collections On Tue, Jan 24, 2012 at 1:18 AM, Salvatore Orlando <salvatore.orla...@eu.citrix.com<mailto:salvatore.orla...@eu.citrix.com>> wrote: Hi Dan, If you are referring to Aaron's comments concerning having distinct plugin calls for filtered list operations, i.e: instead of passing the filter options to get_all_xxx methods, have get_all_xxx_filter methods, it is my opinion that we would need pagination regardless of whether we are filtering or not. Also, just like filtering, pagination might be a lot more efficient if performed by the plugin; for this reason I think that parameter for providing paginated responses should be passed to plugin methods. On the other hand, if you are referring to the comments concerning where these operations (filtering, pagination) should be performed, I'd say that, just as for filtering, we can say that either the plugin supports pagination or it doesn't. If it doesn't we paginate in the API layer. However, in my opinion, we can't get into a situation where a plugin partially supports the feature, as it can happen for filtering: either it can paginate, or it can't. I was talking about the latter. And I agree, pagination would be all or nothing. My only point was that this patch seemed to have a similar question of whether the action should be taken: only outside the plugin, only inside the plugin, or some hybrid. Dan Salvatore PS: The code for JSON serializer was not borrowed, and I haven't written unit tests specific for it. So a detailed review would be welcome, thank you! From: Dan Wendlandt [mailto:d...@nicira.com<mailto:d...@nicira.com>] Sent: 24 January 2012 07:41 To: Salvatore Orlando; Aaron Lee Subject: Re: Change in openstack/quantum[master]: bug/801227 API should support paginated collections On Mon, Jan 23, 2012 at 7:59 AM, Salvatore Orlando (Code Review) <rev...@openstack.org<mailto:rev...@openstack.org>> wrote: Salvatore Orlando has posted comments on this change. Change subject: bug/801227 API should support paginated collections ...................................................................... Patch Set 1: (9 inline comments) I posted replies to Dan's comment inline. The confusion is coming from the fact that this branch does not pass pagination parameters to the plugin. It was not meant to be like this; an updated changeset will be pushed soon. .................................................... File quantum/api/api_common.py Line 197: if not hasattr(self._plugin, 'can_paginate_lists'): I think this check should be performed, because the plugin is actually not required to inherit from the abstract base class. A plugin which just implements the same methods as the ones exposed by quantum_plugin_base will pass all the checks. Ok, that makes sense. If we need this check, I think there are a couple other places in the code where the check needs to be added as well. On the number of items to fetch by default: I have no idea of what can be an ideal value. I think 100 is also the default limit in Openstack. So we can probably go for that. ok, makes sense. As concern the limit and the plugin, the idea is to pass both 'marker' and 'limit' parameters to the plugin, and let the plugin handle them. The approach is similar to api-filters. You won't see this behavior in this changeset thought. I forgot to pass them as parameters to the get_all_xxx plugin methods. Ok, I wonder if Aaron's comments about implementation in the API vs. in the plugin regarding filters also apply to pagination as well. .................................................... File quantum/wsgi.py Line 182: def __init__(self, metadata=None): I agree that I'd love to move to Openstack-common as soon as possible, as this will allow us to get rid of the burden for maintaining this code. This is actually not borrowed from Openstack code, which has a very plain JSON serializer, which just dumps everything. I took it was worth following a better convention for pagination links. The link to the next page is rendered into a "nextLink" element; several restFul APIs, including Google's use this approach. This is something that, in the future, can probably end in openstack-common as well. However, I don't feel this thing as really important for this branch, and I will therefore be happy to revert the JSON serializer to its basic form. No, this makes sense. I was just wondering if I needed to review the code in detail (less necessary if it was code borrowed from an already reviewed code base) Dan -- To view, visit https://review.openstack.org/3141 To unsubscribe, visit https://review.openstack.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5dac90ac02e80a94dafc6f177072f913e071df3a Gerrit-PatchSet: 1 Gerrit-Project: openstack/quantum Gerrit-Branch: master Gerrit-Owner: Salvatore Orlando Gerrit-Reviewer: Salvatore Orlando Gerrit-Reviewer: dan wendlandt <d...@nicira.com<mailto:d...@nicira.com>> -- ~~~~~~~~~~~~~~~~~~~~~~~~~~~ Dan Wendlandt Nicira Networks: www.nicira.com<http://www.nicira.com> twitter: danwendlandt ~~~~~~~~~~~~~~~~~~~~~~~~~~~ -- ~~~~~~~~~~~~~~~~~~~~~~~~~~~ Dan Wendlandt Nicira Networks: www.nicira.com<http://www.nicira.com> twitter: danwendlandt ~~~~~~~~~~~~~~~~~~~~~~~~~~~ -- ~~~~~~~~~~~~~~~~~~~~~~~~~~~ Dan Wendlandt Nicira Networks: www.nicira.com<http://www.nicira.com> twitter: danwendlandt ~~~~~~~~~~~~~~~~~~~~~~~~~~~
-- Mailing list: https://launchpad.net/~netstack Post to : netstack@lists.launchpad.net Unsubscribe : https://launchpad.net/~netstack More help : https://help.launchpad.net/ListHelp
