-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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(postgres) ensure APIs created_at has ms precision #2924
Conversation
Currently in the process of addressing the tests failures related to timestamp precision. |
81fdaa2
to
8f49a05
Compare
uris = { [[/(status)]] }, | ||
upstream_url = helpers.mock_upstream_url .. "/status/200", | ||
}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test tacitly assumes that each entry will take over 1ms to insert?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. This is almost guaranteed to be the case, but I have a feeling we should do something to enforce it. I was thinking of enforcing a "sleep period" subsequently to each API insertion (at the DAO level), but the idea gave me shivers. Any suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd split it into three insert_apis
calls and insert a 2ms ngx.sleep
between each.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do that, but then we'd leave the door open for a disparity between the test case and the actual DAO's behavior. Currently, none of the two introduces a delay, and the test still succeeds. If we add some delay to the test fixtures setup, we might get a false positive result, but conflicting created_at
timestamps might still be possible when using the DAO itself. I feel like we should either insert a delay at the DAO level, either not insert it anywhere to avoid getting a false positive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wouldn't be a false positive: the test would be stating more explicitly what the actual reliability guarantee is. The code would say "if you enter things in different milliseconds, that's the expected behavior".
Not inserting it anywhere means that the test passes by "luck" (and it's a pattern for flaky tests waiting to happen in the future as things get faster).
The delay in the DAO wouldn't protect against ordering issues caused by concurrent access anyway, so to me that's a non-starter (and would give me shivers too!).
In any case, this millisecond approach is an interim hack until proper ordering arrives, so for that purpose it's certainly "good enough". I'd just really prefer if, as a principle, we avoided adding tests whose results depend on the speed of the machine running the test suite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sold! I remembered this morning that we recently switched to using the same approach to reduce some flakiness related to the Targets ordering as well (https://github.com/Mashape/kong/pull/2849/files#diff-dce187085dbbafd3665dbc84cc63500dR294), so we should indeed stay consistent.
The delay in the DAO wouldn't protect against ordering issues caused by concurrent access anyway, so to me that's a non-starter (and would give me shivers too!).
Exactly, little we can do here about it, although generally users run scripts that send concurrent requests to the same node's Admin API. Luckily for us, our new model will have a better chance at dealing with this.
APIs are retrieved without an `ORDER BY` clause from the datastore when building the router. Because of the lack of such a clause in our current Cassandra schema, we wish to implement sorting by creation date in our business logic, right before building the router. However, the PostgreSQL default clause of the APIs `created_at` field is set to `CURRENT_TIMESTAMP(0)`, which strips ms precision out of the timestamp. This changes the default timestamp precision to 3 digits so our sorting attribute (`created_at`) can be meaningful. This fix is targeting 0.11.1, and as such does not introduce a new migration to update the default value of `created_at`. This fix will only have effect for new database schemas. A second commit will introduce a migration, which, as per our policy, will be targeted for 0.12.0. Fix #2899
8f49a05
to
f3ccfc1
Compare
APIs are retrieved without an `ORDER BY` clause from the datastore when building the router. Because of the lack of such a clause in our current Cassandra schema, we wish to implement sorting by creation date in our business logic, right before building the router. However, the PostgreSQL default clause of the APIs `created_at` field is set to `CURRENT_TIMESTAMP(0)`, which strips ms precision out of the timestamp. This changes the default timestamp precision to 3 digits so our sorting attribute (`created_at`) can be meaningful. This fix is targeting 0.11.1, and as such does not introduce a new migration to update the default value of `created_at`. This fix will only have effect for new database schemas. A second commit will introduce a migration, which, as per our policy, will be targeted for 0.12.0. Fix Kong#2899 From Kong#2924
APIs are retrieved without an
ORDER BY
clause from the datastore whenbuilding the router. Because of the lack of such a clause in our current
Cassandra schema, we wish to implement sorting by creation date in our
business logic, right before building the router.
However, the PostgreSQL default clause of the APIs
created_at
fieldis set to
CURRENT_TIMESTAMP(0)
, which strips ms precision out of thetimestamp. This changes the default timestamp precision to 3 digits so
our sorting attribute (
created_at
) can be meaningful.This fix is targeting 0.11.1, and as such does not introduce a new
migration to update the default value of
created_at
. This fix willonly have effect for new database schemas.
A second commit will introduce a migration, which, as per our policy,
will be targeted for 0.12.0.
Fix #2899