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. :)
>
>> 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() ?
>
>> 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.
:).
>
> 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.
>
>> 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.
>
>> 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
Cheers,
— leif
