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(geoprocessing): protected-areas: geo extractor: emit api events #212

Merged
merged 6 commits into from
May 28, 2021

Conversation

kgajowy
Copy link
Contributor

@kgajowy kgajowy commented May 25, 2021

Substitute this line for a meaningful title for your changes

Overview

Please write a description. If the PR is hard to understand, provide a quick explanation of the code.

Designs

Link to the related design prototypes (if applicable)

Testing instructions

Please explain how to test the PR: ID of a dataset, steps to reach the feature, etc.

Feature relevant tickets

Link to the related task manager tickets


Checklist before submitting

  • Meaningful commits and code rebased on develop.
  • If this PR adds feature that should be tested for regressions when
    deploying to staging/production, please add brief testing instructions
    to the deploy checklist (docs/deployment-checklist.md)
  • Update CHANGELOG file

@vercel
Copy link

vercel bot commented May 25, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

marxan – ./app

🔍 Inspect: https://vercel.com/vizzuality1/marxan/FD2miSBkBtimNJRSVPiiCwje9sdD
✅ Preview: https://marxan-git-geoprocessing-marxan-416-api-events-vizzuality1.vercel.app

marxan-storybook – ./app

🔍 Inspect: https://vercel.com/vizzuality1/marxan-storybook/HNK4VqEEga1YZcHe7XtnUTjywmpN
✅ Preview: https://marxan-storybo-git-geoprocessing-marxan-416-api-events-v-9e81ca.vercel.app

Copy link
Member

@hotzevzl hotzevzl left a comment

Choose a reason for hiding this comment

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

thanks @kgajowy.

Besides the minor naming suggestions, I wonder if we should use the data prop of these events to also include in the series of events (submitted/finished or failed) for each request a unique id that links them together, a sort of correlation id. Actually, maybe such correlation id should be a top-level column/prop for API events 🤔...

Probably we can get away without this at this stage - the main issue may be that if users submit two shapefiles for protected areas within a short timeframe and the first one is still processing while the second starts being processed, we may never know of a failure of the second one if the first one takes longer to complete processing and completes successfully (or variations thereof of similar timing issues).

If we feel we should handle these possible race conditions explicitly, we'd still need to handle these correlation ids in a meaningful way, of course.

Comment on lines 7 to 9
('project.wdpa.submitted/v1/alpha'),
('project.wdpa.finished/v1/alpha'),
('project.wdpa.failed/v1/alpha');
Copy link
Member

Choose a reason for hiding this comment

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

I suggest to use protected areas rather than (the more specific) wdpa term where relevant - I know our core db table for protected areas is called wdpa but in practice it's a table for protected area info, some (most in some cases?) of which happen to be from the WDPA source. We've discussed this with Alicia at some point in the past, though we have kept the wdpa table name and some other such references around.

Suggested change
('project.wdpa.submitted/v1/alpha'),
('project.wdpa.finished/v1/alpha'),
('project.wdpa.failed/v1/alpha');
('project.protectedAreas.submitted/v1/alpha'),
('project.protectedAreas.finished/v1/alpha'),
('project.protectedAreas.failed/v1/alpha');


public async down(queryRunner: QueryRunner): Promise<void> {
await queryRunner.query(
`DELETE FROM api_event_kinds WHERE id like 'project.wdpa.%';`,
Copy link
Member

Choose a reason for hiding this comment

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

as above if accepting the previous suggestion

Suggested change
`DELETE FROM api_event_kinds WHERE id like 'project.wdpa.%';`,
`DELETE FROM api_event_kinds WHERE id like 'project.protectedAreas.%';`,

Copy link
Member

Choose a reason for hiding this comment

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

and so on for the rest of the PR 😱 - best to cross check with @aagm that she is indeed happy with this suggestion 😃

Comment on lines 35 to 37
project__wpda__submitted__v1__alpha = 'project.wdpa.submitted/v1/alpha',
project__wpda__submitted__v1__finished = 'project.wdpa.finished/v1/alpha',
project__wpda__submitted__v1__failed = 'project.wdpa.failed/v1/alpha',
Copy link
Member

Choose a reason for hiding this comment

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

please double check the labels of these enum members - did you mean project__wdpa__finished__v1__alpha and so on? (or project__protectedAreas__finished__v1__alpha if replacing wdpa)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦 shame on me! thanks for catching this!

Comment on lines 18 to 20
project__wpda__submitted__v1__alpha = 'project.wdpa.submitted/v1/alpha',
project__wpda__submitted__v1__finished = 'project.wdpa.finished/v1/alpha',
project__wpda__submitted__v1__failed = 'project.wdpa.failed/v1/alpha',
Copy link
Member

Choose a reason for hiding this comment

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

same as in api service

@kgajowy
Copy link
Contributor Author

kgajowy commented May 26, 2021

thanks @kgajowy.

Besides the minor naming suggestions, I wonder if we should use the data prop of these events to also include in the series of events (submitted/finished or failed) for each request a unique id that links them together, a sort of correlation id. Actually, maybe such correlation id should be a top-level column/prop for API events 🤔...

Probably we can get away without this at this stage - the main issue may be that if users submit two shapefiles for protected areas within a short timeframe and the first one is still processing while the second starts being processed, we may never know of a failure of the second one if the first one takes longer to complete processing and completes successfully (or variations thereof of similar timing issues).

If we feel we should handle these possible race conditions explicitly, we'd still need to handle these correlation ids in a meaningful way, of course.

It sounds like a request tracing. If we would like to, consumer (or we at controller level) can assign some transaction-id which is attached to every track-able step (for example, logger in each step) so that we could track full request path.

We can add timestamp / uuid to job as well.

@kgajowy
Copy link
Contributor Author

kgajowy commented May 27, 2021

@hotzevzl fixed the above in 24066f0

@kgajowy kgajowy requested a review from hotzevzl May 27, 2021 07:58
@hotzevzl
Copy link
Member

thanks @kgajowy.
Besides the minor naming suggestions, I wonder if we should use the data prop of these events to also include in the series of events (submitted/finished or failed) for each request a unique id that links them together, a sort of correlation id. Actually, maybe such correlation id should be a top-level column/prop for API events thinking...
Probably we can get away without this at this stage - the main issue may be that if users submit two shapefiles for protected areas within a short timeframe and the first one is still processing while the second starts being processed, we may never know of a failure of the second one if the first one takes longer to complete processing and completes successfully (or variations thereof of similar timing issues).
If we feel we should handle these possible race conditions explicitly, we'd still need to handle these correlation ids in a meaningful way, of course.

It sounds like a request tracing. If we would like to, consumer (or we at controller level) can assign some transaction-id which is attached to every track-able step (for example, logger in each step) so that we could track full request path.

We can add timestamp / uuid to job as well.

I think I overshot by using "correlation id" which indeed is used in request tracing - that's a useful thing to have in general, but here I mean only a way to correlate the ApiEvents related to the same file upload for a specific project. Only something to think about for the reasons/caveats I outlined in my comment, but not for this PR.

@Dyostiq
Copy link
Contributor

Dyostiq commented May 27, 2021

@kgajowy it is not chore :D

@kgajowy kgajowy changed the title chore(geoprocessing): protected-areas: geo extractor: emit api events feat(geoprocessing): protected-areas: geo extractor: emit api events May 27, 2021
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.

3 participants