Simplify tracker implementation

[Code0x58]

The main goal of these changes are to help make the tracker easier to understand and work on in the future. Documentation is automatically generated from the code (thanks to FastAPI) and served at / of running tracker instances. E.g.

heron-tracker --port=8080 &
sleep 5
open http://localhost:8080/

Type annotations are included throughout. Adding mypy to the builds [e.g.] would be good to take advantage of the typing, but it would take additional work due to the pex_* rules instead of regular py_* ones.

A lot of things were touched, but I tried to leave things that looked suspicious but might be relied on as a feature, and didn't intentionally make changes to the interface aside from dropping the redirect at / and some methods returning objects with None values instead of undefined values (hopefully inconsequential - its managable in FastAPI/Pydantic that way). The tests now pass locally.

The API feels a bit rough, for example returning objects with all None values rather than just a None, but I am not addressing things like that now as that would touch additional services.

The commits will be squashed down on merge so I'll clean up the commit message then.

Tracker changes

• requires Python 3.7+ (due to using generic Pydantic model for ResponseEnvelope)
• replace Tornado with FastAPI for self documentation of API
• use async outbound HTTP requests - may give lower latency to some metrics queries
• remove unnecessary use of unittest as pytest is already in use

There's probably still refinement potential, but I'm happy with the improvements to readability.

Exceptions may not appear as graceful in logs, but that's to help highlight where oddities occur so they can be factored out rather than just quietly skipped over.

Behaviour changes

• replace / → /topologies redirect with (OpenAPI on ReDoc) documentation
• deprecate executiontime and always return 0.0 for it in the response envelope - this could probably be put back in, but I don't think it's worth it
• inbound requests aren't terminated if they take over 120s. At the moment, every outbound request has a 5s limit (due to httpx default) so I wouldn't expect the timeout to be needed (even in metrics queries which may transform data). A replacement timeout could be added in if necessary.

To do

This PR:

• review changes
• clean up notes
• revert changes to tools/python/checkstyle.ini (it currently allows TODO/XXX/FIXME)
• update images and docs for the new minimum required version of Python

Follow ons:

• Replace tracker /topologies/{config,physicalplan,executionstate,schedulerlocation,metadata} with /topologies/info #3644 Replace tracker /topologies/{config,physicalplan,executionstate,schedulerlocation,metadata} with /topologies/info
• Factor out passing of strings as exceptions in tracker query_operator.py #3645 Factor out passing of strings as exceptions in tracker query_operator.py

[huijunwu]

'replace Tornado with FastAPI for self documentation of API'
Do we replace Tornado with FastAPI for just Heron-Tracker, or do we plan to replace Tornado for all Heron Python web server?
If I remember correctly, all Heron python webserver align with a single webserver lib/framework, which is Tornado. Curious shall we keep this convention?

[Code0x58]

While cleaning up and adding type annotations to python code in other parts of the codebase I have already been changing Tornado to FastAPI (e.g. in heron ui). The last remaining users of tornado after this are heron shell and one in integration test component.

p.s. on the lines of servers and async in python, heron instance uses asyncore (deprecated) which could probably also benefit from a clean up and migration to asyncio

[nwangtw]

Great! Less dependencies is better~

…

On Sat, Dec 5, 2020 at 1:40 AM Oliver Bristow ***@***.***> wrote: While cleaning up and adding type annotations to python code in other parts of the codebase I have already been changing Tornado to python (e.g. in heron ui <#3597>). The last remaining users of tornado after this are heron shell and one in integration test component. *p.s. on the lines of servers and async in python, heron instance uses asyncore (deprecated) which could probably also benefit from a clean up* — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#3642 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABQPBH57AH2AUKZMEMQX5DLSTH52RANCNFSM4UKP6TZQ> .

[nicknezis]

Is this ready to merge, or do we still need to complete the "To Do" section found in the above description?

[Code0x58]

Is this ready to merge, or do we still need to complete the "To Do" section found in the above description?

I just updated the checklist; the python versions in Dockerfiles needs updating, as well as docs on it - 3.8 works, but 3.7 or 3.8 may be the minimum.

I wouldn't want to merge this at the moment with the Heron 0.20.3 being almost finalised, but it would be good to after. I'd be happiest with someone using this branch in their environment to make sure it works outside of the limited unit+integration tests. When this is confirmed as working, it will be easier to understand what is going on and add tests to cover that functionality over time.

[joshfischer1108]

Yes. Let's hold off on merging until we get 0.20.3-incubating-rc8 fully released. We are still going through the vote process on general@.

[Code0x58]

I rebased this after seeing a PR went in for python 3.8 support, but it looks like the properly supported version isn't that high yet.

There's still the issue of requiring python 3.7+ for this PR. I don't have the time to go through and update and test all of the Docker images which would get this in a good place for testing.

[joshfischer1108]

I rebased this after seeing a PR went in for python 3.8 support, but it looks like the properly supported version isn't that high yet.

There's still the issue of requiring python 3.7+ for this PR. I don't have the time to go through and update and test all of the Docker images which would get this in a good place for testing.

Do you think the work done on this branch is worth one of us following up on to fix the Mac OS build issues related to pex?

[Code0x58]

Do you think the work done on this branch is worth one of us following up on to fix the Mac OS build issues related to pex?

If the core bazel rules required a newer version of Python then that would be a separate line of work to this, which would unblock this PR. I haven't checked in on it so no idea if an upgrade would be needed for it, but could hope not, or that at least it would be apparently very early on from reading bazel's docs.

I don't think there's anything in this PR as is which would help the migration to drop the PEX rules. The closest I can imagine is that this PR is towards removing the last of the tornado requirements, but that shouldn't matter.

[nicknezis]

Closing as #3646 now has the changes from this PR.