search

atlanticwave-sdx / sdx-controller

Central Controller for AtlanticWave SDX.
https://www.atlanticwave-sdx.net
MIT License
1 stars 3 forks source link

change time_stamp to timestamp #190

Closed congwang09 closed 1 year ago

congwang09 commented 1 year ago

This changes time_stamp to timestamp.

Related to: https://github.com/atlanticwave-sdx/sdx-controller/issues/191

coveralls commented 1 year ago

Coverage Status

coverage: 35.919%. remained the same when pulling 0c2d6b5cdd9b94a2c5486e3e4a9b8e0fbee72947 on update-time_stamp-naming into f35a8cd30d712dd5a9d90c0ad62f44b767359ce5 on main.

congwang09 commented 1 year ago

Does this require that the static topology files also will need to change? They all have time_stamp rather than timestamp.

It would be good to have a written record of why this change is required, so that we can find it further down the road.

You mean the ones in pce and datamodel? We probably need to update them as well. Created an issue and linked to the issue in the description. This is to align with the design doc, and timestamp without underscore is usually what we use. I'll create PR for the other two repo as well, and then merge all of them together, just in case something gets broken.

sajith commented 1 year ago

Once https://github.com/atlanticwave-sdx/pce/pull/153 is merged, those JSON files will be in datamodel (packaged as data files, so that pce and sdx-controller can use them), and nowhere else. Perhaps datamodel can also offer a convenience API to access them, if accessing them via an API turns out to be more convenient.

I'm sorry if moving those files once again is a surprise to you! I did not realize that datamodel also needs those files, so they had to be moved one layer down.

There are also schema files and Python code in datamodel (models, handlers/parsers, validators) that use time_stamp instead of timestamp. Also, there are some more annoying details for us to consider:

Our current practices make it kind of hard to keep things in sync. It would be nice to get things a little more streamlined, such that datamodel actually serves its purpose. If I understand its purpose correctly, that is. :-)

I guess https://github.com/atlanticwave-sdx/datamodel/issues/92 can be the meta-issue to track this.

YufengXin commented 1 year ago

sdx-controller imports pce which imports datamodell, right? sdx-lc doesn't use datamodel now, but I wasn't sure if it would in the future if it needs or not. it's a bit annoying. Hopefully after you're done with the current round, we can streamline it.

YufengXin commented 1 year ago

two more comments-:)

  1. While it's annoying, it's currently manageable, even manually, because the data model is not that complex.
  2. I was planning to make PCE a service running in a separate container behind restAPI, because the computation may take some time and block sdx-controller. It's related to the thread issue in sdx-controller

For the near term, I guess we have more burning functional issues to implement, we can leave some of the performance and code structure issues to next year.

sajith commented 1 year ago

It's true that sdx-controller indirectly imports datamodel via pce, but sdx-controller uses its own model modules, not the ones from datamodel. It works okay for now, so making everything "proper" at a later time sounds good.

I hope a nicer solution will emerge as we chip away at the main things SDX should do. It's just that I became confused when I tried to figure out an approach for adding the pieces needed for L2VPN services in pce and datamodel.

YufengXin commented 1 year ago

That's true, the sdx-controller and datamodel need more consolidation to fit in a flow. I could feel your pain when being forced to modifying the model in two places plus the functions in pce, which was I experienced. I believe your current round makes the situation much better.

In this sense, swagger is a bit odd if we need multiple swagger servers that share the same data model/schema. We need a swagger-orchestrator on top of all these services that at least decouple the data model from the server. Maybe there is one open-source somewhere, maybe we can develop one to become rich?-:)

sajith commented 1 year ago

I suspect that a workflow for sharing models across services must exist -- using $ref to an external models.yaml file, perhaps, and this models.yaml could exist in datamodel. We might even be able to use swagger-codegen or openapi-generator in a build or CI step. I just do not have much practical experience with Swagger/OpenAPI to know how well this will work. Also I need to learn the tools. :-)

I guess having models.yaml in datamodel will create a new problem, namely it will slow down API development because the workflow would be a little more complicated.

What we have now should be fine for now. We just need to plan for some restructuring later.