[SFC-164] The OpendaylightSfc singleton should be refactored to work better with the new blueprint changes Created: 21/Sep/16 Updated: 29/May/18 Resolved: 29/May/18 |
|
| Status: | Resolved |
| Project: | sfc |
| Component/s: | General |
| Affects Version/s: | unspecified |
| Fix Version/s: | None |
| Type: | Bug | ||
| Reporter: | Brady Johnson | Assignee: | Guillermo Pablo Tomasini |
| Resolution: | Done | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Environment: |
Operating System: All |
||
| External issue ID: | 6768 |
| Description |
|
Suggestions for improvements by Alexis de Talhouët from this code review: By looking closer at it, it seems this class is providing those things: InstanceIdentifier across the project I think the main concerns of having this class new in every bundle are the close() method and the ExecutorService definition (thread pool of 100). Regarding the close() method, it is well-know that ODL doesn't support uninstall (yet), thus not such a big deal, but it can blow here and there on shutdown because of this. Regarding the ExecutorService, this is more concerning, because you don't want to exhaust the thread pool available in the host running ODL (even though this might not happen, you better be safe). Moreover, the initial intention was to have only one thread pool of 100 threads shared across the whole SFC project. To sum up, I guess for now this is fine, but once all blueprint patches are there, the OpendaylightSfc class needs to be rework and/or break down, and the ExecturoService needs to become shared across all bundle. To do the later, you can defined a FixedThreadPoolWrapper ( https://github.com/opendaylight/controller/blob/master/opendaylight/config/threadpool-config-impl/src/main/java/org/opendaylight/controller/config/threadpool/util/FixedThreadPoolWrapper.java ) in the sfc-provider, and expose the ThreadPool service so it can be gathered from the service registery. |
| Comments |
| Comment by Guillermo Pablo Tomasini [ 27/Sep/16 ] |
|
I have noticed that the wiring in the bundles using "OpendaylightSfc" class is not well done, you should be done by blueprint injection, not manually by the getOpendaylightObj(). I.e sfc-netconf. |
| Comment by Guillermo Pablo Tomasini [ 27/Sep/16 ] |
|
Even within the same bundle sfc-provider the wiring is not quite right, for example some class are pulling from the getter "getOpendaylightSfc()" in order to retrieve an instance of that classs instead of using blueprint injection. I.e: SfcProviderRpc, SfcProviderSfEntryDataListener, SfcProviderSfEntryDataListener, SfcDataStoreAPI, etc. I must fix that wiring in that bundle in order to dispense getOpendaylightSfc. |
| Comment by Guillermo Pablo Tomasini [ 28/Sep/16 ] |
|
SfcDataStoreAPI class (@rpc-provider) must depend on DataBroker Service, no on OpendaylightSfc class as nowadays, I must fix that also in blueprint (rpc-provider). |
| Comment by Guillermo Pablo Tomasini [ 30/Sep/16 ] |
|
I consider that classes instantiated as beans in blueprint should not instantiate another classes with new operator. Instead they should use the blueprint injection (<property name=.... ref=... >). |
| Comment by Guillermo Pablo Tomasini [ 30/Sep/16 ] |
|
There are some classes pulling OpendaylightSfc bean in order of get DataProvider and Broker services, using indirection (through getDataProvider() and getBroker() getters). Why the indirection? if we cant inject both services direct to the beans wich requiere those references. I can fix that. |