Implement a new API called ExecuteR()

[anfredette]

ExecuteR() works the same as the existing Execute() function, except that it returns a list of UUIDs -- one for each object created in the call. If no objects are created in the ExecuteR call, or there is an error, nil is returned. Execute() has been updated to call ExecuteR() and ignore the returned UUIDs to avoid duplication of code.

The reason such a command is useful is that in many cases, the user of our API needs the UUID of the object or objects created. In order to get that UUID, an additional call and in some cases code is needed to get the object and then extract the UUID. Since we already have the UUID in the libovsdb.OperationResult returned from transact, it's more efficient to return it from the Execute call.

[anfredette]

For example, the following list of commands in TestPortGroupACLs():

LSAdd(PG_TEST_LS1)
LSPAdd(PG_TEST_LS1, PG_TEST_LSP1)
LSPSetAddress(PG_TEST_LSP1, ADDR)
LSPSetPortSecurity(PG_TEST_LSP1, ADDR)
LSPAdd(PG_TEST_LS1, PG_TEST_LSP2)
LSPSetAddress(PG_TEST_LSP2, ADDR2)
LSPSetPortSecurity(PG_TEST_LSP2, ADDR2)

Produces the following list of results:

{Count:0 Error: Details: UUID:{GoUUID:a70b2a47-2691-450d-beb5-35584b87b823} Rows:[]}
{Count:0 Error: Details: UUID:{GoUUID:9340bff0-94d4-4ff8-bd15-bb546d0361e1} Rows:[]}
{Count:1 Error: Details: UUID:{GoUUID:} Rows:[]}
{Count:1 Error: Details: UUID:{GoUUID:} Rows:[]}
{Count:1 Error: Details: UUID:{GoUUID:} Rows:[]}
{Count:0 Error: Details: UUID:{GoUUID:3ed1fe23-d5c0-4346-9b5c-2da3174abf70} Rows:[]}
{Count:1 Error: Details: UUID:{GoUUID:} Rows:[]}
{Count:1 Error: Details: UUID:{GoUUID:} Rows:[]}
{Count:1 Error: Details: UUID:{GoUUID:} Rows:[]}

The UUIDs returned are for PG_TEST_LS1, PG_TEST_LSP1, and PG_TEST_LSP2, respectively.

[vtolstov]

nice, but what kind of info i can get from this {Count:1 Error: Details: UUID:{GoUUID:} Rows:[]} ?

[anfredette]

My main goal is to get the UUID(s) as a return value from ExecuteWithResult() when creating new objects. This would avoid having to make a separate call to look up the UUID by name when the UUID is needed by the client. Based on what I'm seeing, we do get UUIDs for successful "create" commands. I was wondering what other useful info we might get but I'm not seeing any.

Other fields seem to contain info for failures. As an example, I mentioned the TestBadTransact() test above; however, transact() currently extracts the info from the first error found and returns it as an Error, so the raw result isn't returned. In the case of TestBadTransact(), the error return is the following:

"Transaction Failed due to an error: constraint violation details: Transaction causes multiple rows in "Encap" table to have identical values (vxlan and "10.0.0.11") for index on columns "type" and "ip". First row, with UUID 7b480885-a953-41d7-9b07-1d8cb3ef6a7c, was inserted by this transaction. Second row, with UUID 4b94492b-911e-44fc-a157-22ecef3b4b45, existed in the database before this transaction and was not modified by the transaction. in committing transaction"

So, my proposal is to just return a list of UUIDs from ExecuteWithResult() when there are no errors. I think this should work based on the following assumptions that would need to be validated:

1. transact() always returns a UUID for new objects created (i.e., new rows inserted in a table)
2. No individual commands result in more than one new row being inserted. (or the behavior is well documented for the user)

[hzhou8]

I think we can keep it like this. It enables further extensibility when needed. And for now, probably the only useful field is the UUID field in the result struct.

For the API name, I am not sure if there is a better choice (something more concise). This one should deprecate the old Execute() API.

[anfredette]

I understand the motivation for extensibility, but I don't like the implementation in my first commit for the reason mentioned in my initial comment:

the challenge is that this list contains a result per operation, and the user only knows about commands, which may contain one or more operations. So that makes it difficult for the user to map between the two.

I've pushed the implementation I'm proposing so it's clear. With the initial implementation, the user would need to know some go-ovn internals to use the results (namely, the fact that some commands use more than one operation), and they'd need to implement code similar to what I've just added to extract the UUIDs.

I'm not crazy about the API name either, but I can't think of a better one right now. I'll think more :-)

[anfredette]

I just thought of another option. If you want to return the results for future extensibility, we could return the result list as in the first commit, and then provide a second helper function to extract the UUIDs from the results using the changes from the second commit. It could be called getUUIDsFromResults() (or something like that.)

[anfredette]

I've updated the code with my proposal for an ExecuteR() function that returns the UUID(s) of object(s) created.

ExecuteR() works the same as the existing Execute() function, except that it returns a list of UUIDs -- one for each object created in the call. If no objects are created in the ExecuteR call, or there is an error, nil is returned. Execute() has been updated to call ExecuteR() and ignore the returned UUIDs to avoid duplication of code.

The reason such a command is useful is that in many cases, the user of our API needs the UUID of the object or objects created. In order to get that UUID, an additional call and in some cases code is needed to get the object and then extract the UUID. Since we already have the UUID in the libovsdb.OperationResult returned from transact, it's more efficient to return it from the Execute call.

The major change from my last commit is that I've renamed the function from ExecuteWithReturn to ExecuteR. (I like this name better, but I'm still open to a better one). I've also added test cases and cleaned up the code.

After my initial test implementation, @hzhou8 suggested that it was fine to return the libovsdb.OperationResult from transact() as-is since this will allow for future extensibility. I see that point, but my thought is that it could be confusing to users of the API since the transact result is an internal data structure and the elements don't map one-to-one with the commands executed because there is one result per operation, and we sometimes use more than one operation commands. In any case, I hope we can resolve this one way or the other and get this API added. Thanks, Andre

[hzhou8]

@vtolstov do you want to check again?

[vtolstov]

No, i think that all fine now.

[anfredette]

Thanks, @hzhou8 and @vtolstov!