weizhouapache commented on PR #9470: URL: https://github.com/apache/cloudstack/pull/9470#issuecomment-2273059362
> Great (and mammoth) work @weizhouapache! I did try to review changes but it is very high-level and may need more eyes. Definitely will need a lot of testing. Great to see many integration tests (we may add some for asnnumber/range) and I hope we will add unit tests at some point as discussed. I've added some comments but may add some more next week. Largely the changes look good. Only some minor concerns: > > * Many new APIs with very similar sounding names. It may get confusing. Maybe if it can be reviewed and consolidated. If not would need a good documentation > * A lot of file changes are due to new arguments in createNetwork, createNetworkOffering, etc methods which could have been possible avoided using overloaded methods. > * While creating API responses we are querying DB multiple times for a single entity. It maybe worth looking if that can be improved either with views or using some `details` parameter in the calling APIs thanks @shwstppr ! I have added the APIs to doc PR: https://github.com/apache/cloudstack-documentation/pull/419 hope users get better understanding I have addressed most of your comments. Regarding the db queries, considering the tables (for ipv4 subnets, bgp peers, as numbers) are comparably very small (with vm_instances, networks or volumes table), we could improve in next stage if users have issues. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@cloudstack.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
