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

Feat/targets to new dao #3622

Closed
wants to merge 8 commits into from
Closed

Feat/targets to new dao #3622

wants to merge 8 commits into from

Conversation

kikito
Copy link
Member

@kikito kikito commented Jul 16, 2018

The new admin API is fairly similar to the previously existing one, but there are some breaking changes:

  • Lists (GET /upstreams or GET /upstreams/{upstream}/targets) don't accept query-attributes for filtering any more
  • Pagination: the total attribute is gone
  • Format of error messages has changed to new DAO errors.
  • PUT actions now are applied to entities instead of to collections. (PUT /upstreams/foo instead of PUT /upstreams
  • Some error codes have changed:
    • Attempting to use an invalid name name (such an IP address) when updating an upstream has become a 500 error instead of a 400.
    • PUT actions return a 200 on creation instead of 201.
  • timestamps (like created_at) are floating point numbers now (in all entities - not just upstreams and services).

New features:

  • There is a attribute called next on paginated admin API results.
  • When listing the targets for an upstream with db.targets:for_upstream(u), the list only includes active targets, and is reverse-sorted by created_at+id. This is the method used in /upstreams/{upstream}/targets
  • There is also a db.targets:for_upstreams_raw(u) which returns all targets, including those with weight=0, associated with the upstream. That's what used in /upstreams/{upstream}/targets/all
  • db.targets:for_upstream_with_health(u) is the same list as db.targets:for_upstream(u) but also includes health information (so it is slower to call). It is used in /upstreams/{upstream}/health.
  • In addition to those there's db.targets:for_upstream_sorted(u) which returns the targets sorted by created_at+id. It is used internally in a couple places for things like cleaning up and building the cache.
  • The db.targets:delete method creates a new target with weight = 0. The only way to really delete a target from the database is by inserting targets (this will eventually trigger a clean_history call).
  • The the db.targets:post_health is the one-place for changing health status of a target. It deals with balancer changes and sending events to the cluster. It is used in POST /upstreams/{upstream}/targets/{target}/healthy|unhealthy.

Prerequisites:

  • Cassandra migrations where not re-entrant (they can not "catch exceptions and continue" like postgres can). In the past the tests where run only in postgresql. I had to add a way to "ignore errors" in order to allow re-entrant migrations in Cassandra.
  • There was a lurking bug related with partial updates of record-type fields.
  • Needed a new kind of validator to allow validating the hash_on_header is not equal to hash_fallback_header
  • The target.target attribute is an endpoint key but it is not unique. Had to remove that restriction from the system.
  • Timestamps should have millisecond-precision, but until we updated the targets to the new DAO we didn't realize that we where storing them with second-precision in the database.

@kikito kikito added the good first issue Issues that beginners/volunteers can easily help with. label Jul 16, 2018
@kikito kikito changed the title Feat/targets to new dao WIP - Feat/targets to new dao Jul 16, 2018
@kikito kikito added pr/do not merge and removed good first issue Issues that beginners/volunteers can easily help with. labels Jul 16, 2018
@thibaultcha thibaultcha added pr/wip A work in progress PR opened to receive feedback and removed pr/do not merge labels Jul 25, 2018
@kikito kikito force-pushed the feat/targets-to-new-dao branch 10 times, most recently from 6424504 to 1837249 Compare July 30, 2018 17:24
@kikito kikito changed the title WIP - Feat/targets to new dao Feat/targets to new dao Jul 30, 2018
@kikito kikito added pr/please review and removed pr/wip A work in progress PR opened to receive feedback labels Jul 30, 2018
@kikito kikito requested a review from hishamhm July 30, 2018 20:17
@kikito kikito force-pushed the feat/targets-to-new-dao branch 4 times, most recently from 0f7289a to 6076cdd Compare August 1, 2018 14:40
@hishamhm
Copy link
Contributor

hishamhm commented Aug 1, 2018

@kikito please remove from this PR the commits that were already merged via other PRs (I'm not doing it myself to avoid getting your local branch out of sync)

@kikito
Copy link
Member Author

kikito commented Aug 2, 2018

@hishamhm done - I have left the timestamps-as-float commit because it is needed for the tests to pass.

@bungle bungle closed this Aug 10, 2018
@bungle bungle deleted the feat/targets-to-new-dao branch August 10, 2018 18:35
sumimakito added a commit that referenced this pull request Aug 23, 2022
This PR fixes the issue that DAO delete hooks were triggered when no rows were affected (rows affected by the DELETE query are unchecked).

The issue currently affects Kong Enterprise and Kong Manager. When concurrent delete requests are initiated on the same entity, the counter in the workspace meta will potentially be decremented multiple times and become a negative number.

FT-3072
sumimakito added a commit that referenced this pull request Aug 24, 2022
This PR fixes the issue that DAO delete hooks were triggered when no rows were affected (rows affected by the DELETE query are unchecked).

The issue currently affects Kong Enterprise and Kong Manager. When concurrent delete requests are initiated on the same entity, the counter in the workspace meta will potentially be decremented multiple times and become a negative number.

FT-3072
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

4 participants