pleroma-elixir-libraries/flake_id
0
0
Fork 0
Code Issues 5 Pull requests 1 Projects Releases Packages Wiki Activity

[WIP] Various improvements and bugfixes #7

Draft
href wants to merge 9 commits from gitlab-mr-iid-2 into master
pull from: gitlab-mr-iid-2
merge into: pleroma-elixir-libraries:master
Conversation 18 Commits 9 Files changed 19 +743 -109
href commented 2020-04-16 17:47:22 +00:00
Member
Copy link
  • Implement in-process generation (fixes #2)
  • Backdated flakes (fixes #1)
  • Persist time for backwards detection (fixes #3)
  • Configurable worker strategy (fixes #4)
  • Follow system clock by using os:system_time instead of erlang:system_time (fixes pleroma/pleroma#1681)
  • Refuse to generate flakes when the clock is not advancing and the sequence overflows
  • Split up building from Worker to a dedicated module Builder
  • Docs improvements
  • tests for local & backdate

BTW I think that once this MR is merged we should release a 1.0, as it's now feature complete and the API won't change.

* [x] Implement in-process generation (fixes #2) * [x] Backdated flakes (fixes #1) * [x] Persist time for backwards detection (fixes #3) * [x] Configurable worker strategy (fixes #4) * [x] Follow system clock by using `os:system_time` instead of `erlang:system_time` (fixes pleroma/pleroma#1681) * [x] Refuse to generate flakes when the clock is not advancing and the sequence overflows * [x] Split up building from `Worker` to a dedicated module `Builder` * [ ] Docs improvements * [x] tests for local & backdate BTW I think that once this MR is merged we should release a 1.0, as it's now feature complete and the API won't change.
kleidukos commented 2020-04-16 23:39:17 +00:00
Member
Copy link
subsequent ones, as it has to set-up the Flake state.
```suggestion:-0+0 subsequent ones, as it has to set-up the Flake state. ```
kleidukos commented 2020-04-16 23:39:36 +00:00
Member
Copy link
when you have long-running processes that do a lot of insertions/ID generation. The first call will be slower than the
```suggestion:-0+0 when you have long-running processes that do a lot of insertions/ID generation. The first call will be slower than the ```
kleidukos commented 2020-04-16 23:45:07 +00:00
Member
Copy link

A couple of typos. Thanks for the work!

A couple of typos. Thanks for the work!
href commented 2020-04-16 23:47:31 +00:00
Author
Member
Copy link

Should we return {:ok, flake} instead of just the flake ? In some cases it could return {:error, ...}. However this will just happen with time being crazy, and will probably crash up the caller later when they do expected a string.

Should we return `{:ok, flake}` instead of just the flake ? In some cases it could return `{:error, ...}`. However this will just happen with time being crazy, and will probably crash up the caller later when they do expected a string.
kleidukos commented 2020-04-16 23:49:27 +00:00
Member
Copy link

If you think that returning {:error, …} is a normal value while operating under normal circumstances, then yes I recommend using {:ok, flakeId} as the return value.

If it is because "Time has gone crazy" and "our dimension is falling appart", I recommend raising an runtime exception and making peace with your Creator.

If you think that returning `{:error, …}` is a normal value while operating under normal circumstances, then yes I recommend using `{:ok, flakeId}` as the return value. If it is because "Time has gone crazy" and "our dimension is falling appart", I recommend raising an runtime exception and making peace with your Creator.
href commented 2020-04-16 23:53:12 +00:00
Author
Member
Copy link

Yes but it can be transient. The clock may go back in the past for a while (like pleroma/pleroma#1681 where the time isn't set correctly when Pleroma starts), but it can be set correctly later. A crash/runtime exception there would make the pleroma service failing (either by refusing to start the flake worker entierly, or by hitting the restart limit from others), while it can resume its work once the clock is set correctly.

Yes but it can be transient. The clock may go back in the past for a while (like pleroma/pleroma#1681 where the time isn't set correctly when Pleroma starts), but it can be set correctly later. A crash/runtime exception there would make the pleroma service failing (either by refusing to start the flake worker entierly, or by hitting the restart limit from others), while it can resume its work once the clock is set correctly.
kleidukos commented 2020-04-16 23:54:11 +00:00
Member
Copy link

Then tuples are the correct way to return a value, if error handling at the library consumer-level makes sense. :)

Then tuples are the correct way to return a value, if error handling at the library consumer-level makes sense. :)
href commented 2020-04-16 23:58:07 +00:00
Author
Member
Copy link

I think so too. This would require an API change. But since it's at 0.1.0 and we're planning 1.0, it's ok, I think.

Also worth noticing for discussion, in pleroma/pleroma#1681, @feld mentioned using +C multi_time_warp. While it is nice, I'd tend against it (and using os:system_time) because: flakes are made to reflect time. The real, human time. Using a time wrap makes it a logical time; and this would require users to set the vm.args properly. On an OTP release well made it's not really an issue, but on dev/home-compiled envs, it is one. We don't want our ids to be possibly wrong because of a small +C issue. Wrap is definitely useful when you have to deal with logical time-- but flakes time isn't one.

I think so too. This would require an API change. But since it's at 0.1.0 and we're planning 1.0, it's ok, I think. Also worth noticing for discussion, in pleroma/pleroma#1681, @feld mentioned using `+C multi_time_warp`. While it is nice, I'd tend against it (and using os:system_time) because: flakes are made to reflect time. The real, human time. Using a time wrap makes it a logical time; and this would require users to set the vm.args properly. On an OTP release well made it's not really an issue, but on dev/home-compiled envs, it is one. We don't want our ids to be possibly wrong because of a small +C issue. Wrap is definitely useful when you have to deal with logical time-- but flakes time isn't one.
lanodan commented 2020-04-17 08:48:34 +00:00
Owner
Copy link

Yeah, huge +1 for returning tuples if it can fail and I'd rather see the API break now to have this than much later on when we're forced to.

Yeah, huge +1 for returning tuples if it can fail and I'd rather see the API break now to have this than much later on when we're forced to.
lambadalambda commented 2020-04-17 11:28:12 +00:00
Owner
Copy link

Follow system clock by using os:system_time instead of erlang:system_time

Wouldn't that break things during daylight saving time changes, when the clock goes backwards?

> Follow system clock by using `os:system_time` instead of `erlang:system_time` Wouldn't that break things during daylight saving time changes, when the clock goes backwards?
href commented 2020-04-17 11:29:52 +00:00
Author
Member
Copy link

It is unix epoch, so UTC/GMT+0, and theres no daylight here

It is unix epoch, so UTC/GMT+0, and theres no daylight here
lambadalambda commented 2020-04-17 11:33:50 +00:00
Owner
Copy link

ah, i get it now. these time options confuse me :)

ah, i get it now. these time options confuse me :)
href commented 2020-04-17 11:39:17 +00:00
Author
Member
Copy link

aaaahhhhh but.

When an UTC leap second is inserted, POSIX time either stops for a second, or repeats the last second. If an UTC leap second would be deleted (which has not happened yet), POSIX time would make a one second leap forward.

But it's probably really an issue either-- if the clock get stuck for a second, we could still generate up to 65536 flakes in that period (the sequence) until it will refuse to generate more (the sequence is 16bit). A worker ips is around ~64k at full speed in a second.

"Repeats the last second" is a bit worrying, not sure I understand it right: would it go backwards like 30 -> 29 or 30 -> 30 ?

aaaahhhhh but. > When an UTC leap second is inserted, POSIX time either stops for a second, or repeats the last second. If an UTC leap second would be deleted (which has not happened yet), POSIX time would make a one second leap forward. But it's probably really an issue either-- if the clock get stuck for a second, we could still generate up to 65536 flakes in that period (the sequence) until it will refuse to generate more (the sequence is 16bit). A worker ips is around ~64k at full speed in a second. "Repeats the last second" is a bit worrying, not sure I understand it right: would it go backwards like 30 -> 29 or 30 -> 30 ?
lambadalambda commented 2020-04-17 11:42:13 +00:00
Owner
Copy link

maybe we need something like https://erlang.org/doc/man/erlang.html#monotonic_time-0

maybe we need something like https://erlang.org/doc/man/erlang.html#monotonic_time-0
href commented 2020-04-17 11:47:37 +00:00
Author
Member
Copy link

yeah, that could work, but that would break the fact that flakes are human time based, and sorted on human time: it's a logical clock. We won't be able to match a flake to a point in time. "since some unspecified point in time", "Different runtime system instances will use different unspecified points in time as base for their Erlang monotonic clocks"; "it is pointless comparing monotonic times from different runtime system instances".

yeah, that could work, but that would break the fact that flakes are human time based, and sorted on human time: it's a logical clock. We won't be able to match a flake to a point in time. "since some unspecified point in time", "Different runtime system instances will use different unspecified points in time as base for their Erlang monotonic clocks"; "it is pointless comparing monotonic times from different runtime system instances".
lambadalambda commented 2020-04-17 11:51:14 +00:00
Owner
Copy link

oof

oof
href commented 2020-04-17 11:51:19 +00:00
Author
Member
Copy link

Apparently POSIX CLOCK_MONOTONIC cannot go backwards-- but I'm not sure if Erlang is using this one. i'll continue digging

Apparently POSIX CLOCK_MONOTONIC cannot go backwards-- but I'm not sure if Erlang is using this one. i'll continue digging
href commented 2020-05-12 20:55:02 +00:00
Author
Member
Copy link

so huh basically time is so fucked up! to not fuck up with leap seconds etc, it'll definitely need the multi time warp mode to be enabled. Added a warning at flake's startup if it's not enabled.

so huh basically time is so fucked up! to not fuck up with leap seconds etc, it'll definitely need the multi time warp mode to be enabled. Added a warning at flake's startup if it's not enabled.
* Implement in-process generation (cc #2)
* Backdated flakes (cc #1)
* Persist time for backwards detection (still wip, cc #3)
* Configurable worker strategy (still wip, cc #4)
* Follow system clock by using `os:system_time` instead of
`erlang:system_time` (fixes pleroma/pleroma#1681)
* Refuse to generate flakes when the clock is not advancing and the
sequence overflows
* Split up building from `Worker` to a dedicated module `Builder`
This pull request is marked as a work in progress.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin gitlab-mr-iid-2:gitlab-mr-iid-2
git switch gitlab-mr-iid-2

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch master
git merge --no-ff gitlab-mr-iid-2
git switch gitlab-mr-iid-2
git rebase master
git switch master
git merge --ff-only gitlab-mr-iid-2
git switch gitlab-mr-iid-2
git rebase master
git switch master
git merge --no-ff gitlab-mr-iid-2
git switch master
git merge --squash gitlab-mr-iid-2
git switch master
git merge --ff-only gitlab-mr-iid-2
git switch master
git merge gitlab-mr-iid-2
git push origin master
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
pleroma-elixir-libraries/flake_id!7
No description provided.