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

change time_stamp to timestamp #190

Merged
merged 1 commit into from
Sep 7, 2023
Merged

Conversation

congwang09
Copy link
Contributor

@congwang09 congwang09 commented Aug 16, 2023

This changes time_stamp to timestamp.

Related to: #191

@coveralls
Copy link

Coverage Status

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

Copy link
Member

@sajith sajith left a comment

Choose a reason for hiding this comment

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

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.

@congwang09
Copy link
Contributor Author

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
Copy link
Member

sajith commented Aug 16, 2023

Once atlanticwave-sdx/pce#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:

  • It does not look like sdx-controller and sdx-lc uses the models from datamodel, at the moment. (There are issues about this: Use models from datamodel #42, Use models from datamodel sdx-lc#50)
  • It also looks like there's a bit of circular relationship between datamodel and the applications, because the models were generated by Swagger/OpenAPI tools, and because of the fact that the definitions are in sdx-controller and sdx-lc, and the generated code is copied over to datamodel.

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 atlanticwave-sdx/datamodel#92 can be the meta-issue to track this.

@YufengXin
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Member

sajith commented Aug 17, 2023

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
Copy link
Collaborator

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
Copy link
Member

sajith commented Aug 17, 2023

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.

@congwang09 congwang09 merged commit 140eb75 into main Sep 7, 2023
5 checks passed
@congwang09 congwang09 deleted the update-time_stamp-naming branch September 7, 2023 14:47
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.

4 participants