> On Jun 10, 2016, at 3:19 PM, Leif Hedstrom <zw...@apache.org> wrote: > > Thanks James, as usual great review. Replies inline. > >> On Jun 10, 2016, at 2:30 PM, James Peach <jamespe...@me.com> wrote: >> >> >>> On Jun 10, 2016, at 10:29 AM, Leif Hedstrom <zw...@apache.org> wrote: >>> >>> Hi, >>> >>> as part generalizing the concept from >>> https://github.com/apache/trafficserver/pull/199 >>> <https://github.com/apache/trafficserver/pull/199>, I’ve filed a Jira to >>> add appropriate APIs and I_Machine.h / Machine.cc <http://machine.cc/> >>> support for the concept of a process UUID. The apidefs.h are >>> >>> /* >>> -------------------------------------------------------------------------- >>> Interface for the UUID APIs. https://www.ietf.org/rfc/rfc4122.txt >>> <https://www.ietf.org/rfc/rfc4122.txt> . */ >>> typedef enum { >>> TS_UUID_V1 = 1, >>> TS_UUID_V2, >>> TS_UUID_V3, >>> TS_UUID_V4, /* At this point, this is the only implemented version (or >>> variant) */ >>> TS_UUID_V5, >>> } TSUuidVersion; >>> >>> #define TSUuidStringLen 36 >> >> Is this define useful? I think we should drop it ... in libuuid it is mainly >> used for formatting a string and this API doesn't need that. > > Yeah, I’m fine with that, the only time it’d be useful is if you want to > allocate something on the stack to hold a copy I think, or want to avoid > doing a strlen() for e.g. a malloc. No strong opinion from me. :)
That makes sense. It would be useful for TSUuidStringParse too I think. >>> typedef struct tsapi_uuid *TSUuid; >>> >>> >>> The ts.h APIs are: >>> >>> /* APIs for dealing with UUIDs, either self made, or the system wide >>> process UUID. */ >>> tsapi TSUuid TSUuidCreate(TSUuidVersion v); >> >> A helpful extension of this might be TSHttpTxnUuidCreate() which would >> allocate the UUID in the transaction arena. > > Yep. I’ll file a Jira if no one else beats me to it. > > >> >>> tsapi TSReturnCode TSUuidDestroy(TSUuid uuid); >>> tsapi TSReturnCode TSUuidCopy(TSUuid dest, TSUuid src); >>> tsapi TSReturnCode TSUuidInitialize(TSUuid uuid); /* Re-generate the UUID */ >> >> If you TSUuidCreate() but don't TSUuidInitialize(), what do you get? > > It’ll still be initialized. The Initialize is really just a “re-initialize”, > to get a new, random UUID. I don’t know how useful it is, but it’s also > harmless. Maybe we need a better name? TSUuidReinitialize() ? Ok, do we need this at all then? I am not sure why I would get a UUID and then change its value? >>> tsapi const TSUuid TSProcessUuidGet(); >> >> Is this persistent across restarts? As we discussed on IRC, it would be good >> to expose this value in metrics and traffic_ctl. > > No, not persistent. I added the metric as discussed, but if we need more > oomph in traffic_ctl, lets file a separate Jira. > > For persistent UUID, I’m ok adding that as well, although sort of feels that > the “hostname” is that unique, persistent ID. > > >> >>> tsapi const char *TSUuidStringGet(const TSUuid uuid); >> >> Does this return an allocated string? > > No, it’s part of the UUID object. It has the same lifetime as the underlying > TSUuid object (which is forever in the case of the Process UUID). I’ll make > sure to document this properly. > > To be clear, there is no transfer of ownership here, you just get a pointer > right into the implementations data. If we feel strongly, we can change that, > but that makes for more complex APIs, and not sure of the value. We do the > exact same things with most strings, to avoid unnecessary copies in our APIs. > :). That makes sense. >> Consider TSUuid TSUuidStringParse() as an additional API. > > > Roger. So, > > if (TS_SUCCESS == TSUuidStringParse(uuid, > "8E3C2CA3-3603-4A85-A814-8415F395D904”)) { …}, > > right ? Do we want an int len third (optional) parameter? For the case of > non-null terminated strings. I don’t think you need the length since the string is known fixed length. uuid_parse(3) doesn’t use it either. Though if there’s no length parameter maybe we should add a constant for the length so the caller can check? >>> and an additional API to expose the already existing HttpSM (Txn) sequence >>> ID: >>> >>> /* Get the Txn's (HttpSM's) unique identifier, which is a sequence number >>> since server start) */ >>> tsapi int64_t TSHttpTxnIdGet(); >> >> Why int64_t rather than uint64_t? > > Cause HttpSM.sm_id is int64_t :-). We’d have to change things in a lot of > places for that to be addressed, which we can do if you feel strongly, but > it’s a fair amount of work. Yeh I think we should do this. uint64_t wraps nicely. Just cast it for this API, then come back and change it everywhere. >>> The APIs are documented in a separate Sphinx file, i.e. no doxygen crud in >>> the ts.h file :). >> >> The documentation should be posted with the review to save a lot of >> redundant questions. > > Agreed. I’m in a special chicken-and-egg situation, so don’t do what I do :-/. > > New metric that was added per the IRC discussions: > > [root@fedora ats]# ./bin/traffic_ctl metric match uuid > proxy.process.version.server.uuid 42f4da08-29cf-4a93-942a-d72eb526d18d Nice. J