> 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

Reply via email to