> On Jun 11, 2016, at 11:07 AM, James Peach <[email protected]> wrote:
>
>>
>> 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?
The thinking was if you want to generate a new UUID many times, without the
allocations necessary to get the “generator” object.
Now, after we added the TSUuidStringParse() function (done), I started thinking
that maybe a cleaner API is something in the line of (without error checking):
uuid = TSUuidCreate();
TSUuidInitialize(uuid, TS_UUID_V4);
and
uuid = TSUuidCreate();
TSUuidStringParse(uuid, "65c65060-072e-4f9c-a76f-89f5f245354a”); // The
string holds the version, so no issue there
This definitely seems cleaner to me, specially now that we have at least two
ways of “generating” the actual UUID? Thoughts?
>>
>> 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?
Ok, so we agree on:
tsapi TSReturnCode TSUuidStringParse(TSUuid uuid, const char *uuid_str);
>
>>>> 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.
I doubt anyone will ever change the code, and this is borderline bike shedding;
It’s clearly a non issue, since we’ll never, ever wrap this ID. Even at
300,000 QPS, it’d take a million years to wrap it. But, I don’t really care, so
I’ll change it to uint64_t.
Cheers,
— leif
