Page MenuHomePhabricator

AQS is not OpenAPI 3 compliant
Open, MediumPublic

Description

In order to upgrade service-runner and and hyperswitch to more recent versions, AQS OpenAPI definition should be upgraded to be OpenAPI 3 compliant.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 17 2019, 9:08 PM

Change 558685 had a related patch set uploaded (by Cwhite; owner: Cwhite):
[analytics/aqs@master] update openapi definitions to version 3

https://gerrit.wikimedia.org/r/558685

fdans triaged this task as Medium priority.Dec 23 2019, 4:40 PM
fdans moved this task from Incoming to Analytics Query Service on the Analytics board.
Nuria added subscribers: Pchelolo, mforns, Nuria.EditedSep 15 2020, 10:51 PM

ping @Pchelolo and @mforns that were mentioning this today, we should probably schedule this work, correct?

@colewhite I see the patch has no reviewers, should we pick it up?

By all means. The patch was generated by a tool, and I applied some manual stylistic formatting you may or may not want. Have a look and do with it what you see fit.

Maybe @paulkernfeld is interested on this ticket?

We probably need to start patch from scratch

Yeah, I can take a look at this. I have a couple questions:

  • What version of node and npm do you use to generate package.json? When I try with node 10.22.1 and npm 6.14.6, the package.json changes slightly, for example. I don't want to cause unnecessary noise in the diffs.
  • When you say to start from scratch, would that be re-running swagger2openapi from the head of master? Or are there other things to fix?
  • It looks like the existing patch passes unit tests, but should I add any new tests?

I would suggest waiting a little on this. AQS is based on a very outdated codebase of RESTBase/hyperswitch that do not support openAPI 3 yet, and upgrading to a newer version will be a much larger endeavor.

Bigger picture: we want to retire RESTBase (the service) and replace it with envoy, shifting most on the RESTBase responsibilities into the services it's proxying. For AQS RESTBase is just a proxy, it's not really doing anything except publicly exposing it, so moving AQS from behind RESTBase will be trivial. However, when we do retire restbase, we hope to deprecate/archive it's codebase which AQS is based on. Given how trivial AQS is, I was going to propose to rewrite it from scratch in go, it would probably be less then a week-long project. If that plan is agreed upon, there will be no reason to spend any time on the current AQS codebase.

I'm going to come with a more formal proposal next week probably.

Nuria added a comment.Sep 17 2020, 3:17 PM

i see, @paulkernfeld this is not a best fit then. Unassigning.

Nuria added a comment.Sep 17 2020, 4:21 PM

Given how trivial AQS is, I was going to propose to rewrite it from scratch in go,

AQS talks to druid and cassandra and there is a bit of "meat" on the metric side on the "routing" side yes, the code is trivial

So, why Go? Are we completely moving away from node? Will there be a new version of node-service-template?

I think we can easily migrate to a new version of node-service-template but I think we would need a clear value proposition to rewrite stuff like this. It's trivial, just composing druid requests, but working with minimal maintenance and would probably take longer than a week to rewrite with all the unit tests and integration tests.

service-template-node could be another possibility, yes. The point is that it's just shouldn't rely on restbase codebase anymore. I'll come up with a more detailed proposal soon.