> On Jun 10, 2016, at 3:19 PM, Leif Hedstrom <[email protected]> wrote:
>
> Thanks James, as usual great review. Replies inline.
>
>> On Jun 10, 2016, at 2:30 PM, James Peach <[email protected]> wrote:
>>
>>
>>> On Jun 10, 2016, at 10:29 AM, Leif Hedstrom <[email protected]> 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
