Start Jetty server in BGP example#2447
Conversation
Summary of ChangesHello @PeterSuna, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical initialization step for the BGP example application. It ensures that the Restconf API is properly enabled and accessible by explicitly starting the embedded Jetty server. Without this change, the Restconf API would not be operational, hindering the application's ability to interact via its intended interface. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a bug in the BGP example application where the Jetty server for Restconf was not being started, making the API inaccessible. The changes involve updating the restconf module's type to its concrete implementation and adding the necessary startServer() call. While the fix is correct, I've added a comment regarding a potential design improvement in the CommunityRestConf module to make its startup process more aligned with the LightyModule interface, which would improve long-term maintainability.
| .build(); | ||
| Preconditions.checkState(startLightyModule(restconf, modulesConfig.getModuleTimeoutSeconds()), | ||
| "Unable to start restconf module"); | ||
| restconf.startServer(); |
There was a problem hiding this comment.
Calling startServer() here fixes the issue of the Restconf API not being available. However, the need to call this method separately after startLightyModule() suggests a design issue in CommunityRestConf.
The LightyModule interface contract implies that the start() method should be sufficient to make a module operational. By requiring an additional startServer() call, CommunityRestConf creates a "leaky abstraction," forcing consumers to be aware of its internal startup sequence. This is why the restconf field's type had to be changed from LightyModule to the concrete CommunityRestConf.
For better maintainability and adherence to the LightyModule contract, the logic within startServer() should ideally be part of the CommunityRestConf.initProcedure() method. This would make the module's startup atomic from the consumer's perspective and avoid the need for casting to a concrete type.
While changing CommunityRestConf is likely out of scope here, it's worth considering a follow-up to refactor it for a cleaner API.
There was a problem hiding this comment.
@PeterSuna create a ticket for this fix, and a separate one with suggestion above.
| .build(); | ||
| Preconditions.checkState(startLightyModule(restconf, modulesConfig.getModuleTimeoutSeconds()), | ||
| "Unable to start restconf module"); | ||
| restconf.startServer(); |
There was a problem hiding this comment.
@PeterSuna create a ticket for this fix, and a separate one with suggestion above.
Without this Jetty initialization, we will not be able to use Restconf API. JIRA: LIGHTY-397 Signed-off-by: Peter Suna <[email protected]>
PeterSuna
force-pushed
the
fixBgpRestcont
branch
from
c002ecd to
27011a3
Compare
2 participants
Without this Jetty initialization, we will not be able to use Restconf API.
JIRA: LIGHTY-397