OpenJPA
  1. OpenJPA
  2. OPENJPA-361

Incorrect GREG_OFFSET offset or inconsistent usage in UUIDGenerator

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 1.0.0
    • Fix Version/s: 1.0.1, 1.1.0
    • Component/s: lib
    • Labels:
      None
    • Environment:
      All platforms

      Description

      In UUIDGenerator, GREG_OFFSET is defined as

      // offset to move from 1/1/1970, which is 0-time for Java, to gregorian
      // 0-time 10/15/1582, and multiplier to go from 100nsec to msec units
      private static final long GREG_OFFSET = 0x01b21dd213814000L;

      This constant is used as:

      // calculate time as current millis plus offset times 100 ns ticks
      long currentTime = (_currentMillis + GREG_OFFSET) * MILLI_MULT;

      Based of the usage, GREG_OFFSET should be in msec unit and this value should be

      (1970-1582) * 365.25 * 24 * 3600 * 1000 (ms) = 12,219,292,800,000 (ms) = 0xB1D 069B 5400 (ms)

      The defined constant value 0x1b21dd213814000L is the last value in 100ns unit not ms.

      This also offsets the currentTime calculation off by a factor of 10**4.

      To correct the discrepency, either:

      1) GREG_OFFSET should assign the value of 0xB1D 069B 5400L instead of 0x1b2 1dd2 1381 4000L, or
      2) long currentTime = (_currentMillis * MILLI_MULT ) + GREG_OFFSET ;

      Since UUID is an arbituary value, therefore I don't believe the current implementation is "incorrect" but just inconsistent in its implementation description.

      Please comment on if:
      1) the suggested change will have any undesirable side-effect for UUID
      2) there is any legacy/backward compatibility problem
      3) this is worth to change at all.

      If there is no objection, I'll correct the "problem" early next week.

      Albert Lee.

        Activity

        Hide
        Albert Lee added a comment -

        Change GREG_OFFSET value to match the description.

        Show
        Albert Lee added a comment - Change GREG_OFFSET value to match the description.
        Hide
        Kevin Sutter added a comment -

        Looks like a good catch, Albert. I went back and looked at the Apache Sandbox code that this implementation was based off of and, sure enough, the constant for GREG_OFFSET should have been 12,219,292,800,000 (ms) or 0xB1D 069B 5400 (ms). We should go ahead with this change.

        Show
        Kevin Sutter added a comment - Looks like a good catch, Albert. I went back and looked at the Apache Sandbox code that this implementation was based off of and, sure enough, the constant for GREG_OFFSET should have been 12,219,292,800,000 (ms) or 0xB1D 069B 5400 (ms). We should go ahead with this change.
        Hide
        Patrick Linskey added a comment -

        I'm concerned about the backward-compat issue, but haven't done any analysis of it.

        Show
        Patrick Linskey added a comment - I'm concerned about the backward-compat issue, but haven't done any analysis of it.
        Hide
        Kevin Sutter added a comment -

        > I'm concerned about the backward-compat issue, but haven't done any analysis of it.

        I don't think we have anything to worry about. Looking at the incorrect value that was being used for the Gregorian Offset, we were basically adding somewhere around 380 years to the current time. So, the possibility of someday matching up a new generated UUID (with the correct Gregorian offset) with an old one is next to nil. Especially when you bring the other aspects of the UUID into play (IP, clock sequence, etc).

        I'm going to go ahead with this commit.

        Kevin

        Show
        Kevin Sutter added a comment - > I'm concerned about the backward-compat issue, but haven't done any analysis of it. I don't think we have anything to worry about. Looking at the incorrect value that was being used for the Gregorian Offset, we were basically adding somewhere around 380 years to the current time. So, the possibility of someday matching up a new generated UUID (with the correct Gregorian offset) with an old one is next to nil. Especially when you bring the other aspects of the UUID into play (IP, clock sequence, etc). I'm going to go ahead with this commit. Kevin
        Hide
        Patrick Linskey added a comment -

        Ok. But in 380 years, I'm going to refer all bug reports about this to you.

        Show
        Patrick Linskey added a comment - Ok. But in 380 years, I'm going to refer all bug reports about this to you.
        Hide
        Kevin Sutter added a comment -

        Resolved in both trunk and the 1.0.x branch.

        Show
        Kevin Sutter added a comment - Resolved in both trunk and the 1.0.x branch.

          People

          • Assignee:
            Albert Lee
            Reporter:
            Albert Lee
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development