Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix erlang time module compatibility #883

Merged
merged 1 commit into from
Oct 11, 2017

Conversation

nickva
Copy link
Contributor

@nickva nickva commented Oct 9, 2017

Fix erlang time module compatibility

`now/0` is deprecated since Erlang 18.0, and a set of new time related
functions are available.

Usually `now/0` can be replaced with `os:timestamp/0`, however in some
instances it was used effectively to produce monotonically incrementing values
rather than timestamps. So added a new `couch_util:unique_monotonic_integer/0`.

Most functional changes are in couch_uuid module. There `now/0` was used both
as a timestamp and for uniqueness. To emulate previous behavior, a local
incrementing clock sequence is used. If `os:timestamp/0` does not advance since
last call then the local clock is advanced by 1 microsecond and that's used to
generate the next V1 UUIDs. As soon as os:timestamp/0` catches up, the local
sequence reset to that latest value.

Also exported function `utc_random/0` was not used, after updating the function
it wasn't exported anymore.

@nickva nickva force-pushed the time-compatibility-module branch 2 times, most recently from f2a6fec to 52858c0 Compare October 10, 2017 15:04
Copy link
Member

@davisp davisp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, looks like I never submitted this PR review. So here it is.

@@ -203,7 +203,8 @@ handle_call({fetch, UserName}, _From, State) ->
[] ->
couch_stats:increment_counter([couchdb, auth_cache_misses]),
Creds = get_user_props_from_db(UserName),
State1 = add_cache_entry(UserName, Creds, erlang:now(), State),
ATime = couch_time_compat:unique_integer([monotonic, positive]),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this would be more useful as couch_util:unique_integer() that gets left forever rather than replacing all of these with erlang:unique_integer/1 in the future (we do this for a few different things where we always set the same options if memory serves).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put it here mainly to hide the ifdef ugliness. But I can move it to couch_util

Though we should probably rename it as well to unique_monotonic_integer since unique_integer/0 is not guaranteed to be always increasing, in theory could jump around like -10, then 100, then -5 etc.

list_to_binary(Prefix ++ Suffix).
Suffix = couch_util:to_hex(crypto:strong_rand_bytes(9)),
Now = os:timestamp(),
{UtcRandom, _NextClockSeq} = utc_suffix(Suffix, 0, Now),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% on whether we should be hard coding a 0 here for anyone that calls this or if we should be funneling every call through the gen_server (and thus have to change the entire gen_server to always maintain a clock sequence).

Also, near as I can tell exactly zero calls are made to utc_random in the source code which may inform our decision on this bit.

Copy link
Contributor Author

@nickva nickva Oct 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought utc_random could be selected as one of the algorithms in the config. But you're right, it should probably get the same local clock sequence treatment as the utc_id

Copy link
Member

@davisp davisp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

after my super minor nit.

Then = calendar:datetime_to_gregorian_seconds({{1970, 1, 1}, {0, 0, 0}}),
Prefix = io_lib:format("~14.16.0b", [(Nowsecs - Then) * 1000000 + Micro]),
list_to_binary(Prefix ++ Suffix).
utc_random(ClockSeq) ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nits but you should move this to after the gen_server behavior callbacks.

Copy link
Contributor Author

@nickva nickva Oct 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. Will move utc_suffix as well since it is not exported.

`now/0` is deprecated since Erlang 18.0, and a set of new time related
functions are available.

Usually `now/0` can be replaced with `os:timestamp/0`, however in some
instances it was used effectively to produce monotonically incrementing values
rather than timestamps. So added a new `couch_util:unique_monotonic_integer/0`.

Most functional changes are in couch_uuid module. There `now/0` was used both
as a timestamp and for uniqueness. To emulate previous behavior, a local
incrementing clock sequence is used. If `os:timestamp/0` does not advance since
last call then the local clock is advanced by 1 microsecond and that's used to
generate the next V1 UUIDs. As soon as os:timestamp/0` catches up, the local
sequence reset to that latest value.

Also exported function `utc_random/0` was not used, after updating the function
it wasn't exported anymore.
@nickva nickva merged commit a99cc6f into apache:master Oct 11, 2017
@nickva nickva deleted the time-compatibility-module branch October 11, 2017 01:38
janl added a commit to janl/couchdb that referenced this pull request Oct 28, 2017
janl added a commit that referenced this pull request Oct 29, 2017
wohali pushed a commit that referenced this pull request Oct 31, 2017
wohali pushed a commit that referenced this pull request Nov 1, 2017
wohali pushed a commit that referenced this pull request Nov 1, 2017
lucidNTR added a commit to cloudless-hq/couchdb that referenced this pull request Jan 2, 2018
…-search

* commit 'f84faa532ee7341d76f7cecdd416f8728d72ecc1': (4288 commits)
  re-add query
  Update NOTICE: remove autoconf/m4 references and revert react patents note
  Configurable delay before retrying on missing_doc error
  Update jiffy to use dedupe_keys
  Return error 410 on temporary view request
  fix(peruser_test): on slow CI vms, we can get Cluster timeouts
  Updating verifyinstall URL
  whitespace
  add delay for bulk_delete and create
  up delay
  use delay instead
  use string for w value
  Update w value for deletion since n=1
  feat: port time funs, as per apache#883
  fix: peruser tests: use spinlocks instead of timer:sleep()
  Alias /_node/_local/... to /_node/<nodename>@<hostname>/...
  Travis: s/20.0/20.1 for build
  Disable eval() and Function() constructor in JS by default
  Blacklist some config sections from HTTP PUT/DELETE operations
  Cleanups for 2.1.1 proper version tagging
  ...

# Conflicts:
#	Makefile
#	Makefile.win
#	dev/run
#	rebar.config.script
#	rel/reltool.config
willholley pushed a commit to willholley/couchdb that referenced this pull request May 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants