ref(subscriptions): Decouple subscription logic from datasets [INGEST-627] #2258
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## cra-metrics/add-metrics-counters-entity-subscription #2258 +/- ##
========================================================================================
- Coverage 93.19% 93.18% -0.01%
========================================================================================
Files 552 552
Lines 25106 25150 +44
========================================================================================
+ Hits 23397 23437 +40
- Misses 1709 1713 +4
Continue to review full report at Codecov.
|
lynnagara
reviewed
Dec 8, 2021
| @@ -435,25 +438,49 @@ def handle_subscription_error(exception: InvalidSubscriptionError) -> Response: | |||
| ) | |||
|
|
|||
|
|
|||
| @application.route("/<dataset:dataset>/<entity:entity>/subscriptions", methods=["POST"]) | |||
| @application.route("/<dataset:dataset>/subscriptions", methods=["POST"]) | |||
There was a problem hiding this comment.
Are you planning to remove this one as a follow up? I think we should
There was a problem hiding this comment.
Yep that's the plan, I am planning to remove line 442 and 470 i.e. for creation and deletion as a follow up
There was a problem hiding this comment.
Sounds good, it could be useful to mark or comment these in code so it's clear what will happen to the old routes
lynnagara
reviewed
Dec 8, 2021
Comment on lines
469
to
498
| @application.route( | ||
| "/<dataset:dataset>/subscriptions/<int:partition>/<key>", methods=["DELETE"] | ||
| ) |
There was a problem hiding this comment.
Same here, let's deprecate this one once it is no longer being used?
|
Just some questions about which endpoints we'll be deprecating, otherwise LGTM |
lynnagara
approved these changes
Dec 8, 2021
ahmedetefy
force-pushed
the
cra-metrics/add-metrics-counters-entity-subscription
branch
3 times, most recently
from
c07e832 to
efab254
Compare
ahmedetefy
force-pushed
the
cra-metrics/decouple-dataset-subscriptions
branch
from
0873d16 to
3dc8290
Compare
ahmedetefy
force-pushed
the
cra-metrics/add-metrics-counters-entity-subscription
branch
from
efab254 to
26ac38c
Compare
ahmedetefy
force-pushed
the
cra-metrics/decouple-dataset-subscriptions
branch
2 times, most recently
from
701f26f to
60a874f
Compare
ahmedetefy
force-pushed
the
cra-metrics/add-metrics-counters-entity-subscription
branch
from
3fa7c5f to
fc58934
Compare
ahmedetefy
force-pushed
the
cra-metrics/decouple-dataset-subscriptions
branch
from
60a874f to
1ca7c85
Compare
Base automatically changed from
cra-metrics/add-metrics-counters-entity-subscription
to
master
December 15, 2021 14:36
…-627] (#2244) * ref(subscriptions): Decouple subscription logic from datasets Decouples subscription creation and deletion logic from datasets to also consider entities since dataset to entity is not a 1-to-1 relation * Add test that ensures selected dataset is actually being used
ahmedetefy
force-pushed
the
cra-metrics/decouple-dataset-subscriptions
branch
from
1ca7c85 to
a01c23c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replica of #2244
Re-opening because I accidentally merged it into the main branch