MDSAL best practices
(NETVIRT-1318)
|
|
| Status: | Resolved |
| Project: | netvirt |
| Component/s: | elanmanager |
| Affects Version/s: | None |
| Fix Version/s: | Fluorine-SR1, Neon |
| Type: | Sub-task | Priority: | Medium |
| Reporter: | Stephen Kitt | Assignee: | Manu B |
| Resolution: | Done | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Comments |
| Comment by Vishal Thapar [ 02/Jul/18 ] |
|
ezghodh If you're not actively working on this please unassign yourself so someone else can work on this. |
| Comment by Manu B [ 04/Jul/18 ] |
|
Review added doesn't use DS constraint txn at all the places. It is difficult to add where same transaction is used for both config and oper. Similarly it is difficult to use for places where blocking calls are used. |
| Comment by Stephen Kitt [ 04/Jul/18 ] |
|
Could you give an example where blocking calls make it difficult to use? |
| Comment by Manu B [ 04/Jul/18 ] |
|
For eg: deleteElanInterfaceForwardingTablesList in ElanForwardingEntriesHandler.java is called from 2 places( one from deleteElanInterfaceForwardingEntries and updateElanInterfaceForwardingTablesList from same file). It is easy to move to DS constraint tx in deleteElanInterfaceForwardingEntries but difficult at updateElanInterfaceForwardingTablesList. This is because updateElanInterfaceForwardingTablesList is called from update method of ElanInterfaceManager.java which doesn't use txRunner(due to blocked call). This resulted in not using DS constraint txn in both deleteElanInterfaceForwardingEntries and updateElanInterfaceForwardingTablesList. Please let me know if there is any better way to use it.
|
| Comment by Stephen Kitt [ 04/Jul/18 ] |
|
The better way is to use an existing transaction in `ElanInterfaceManager::update`, without blocking. (Blocking shouldn’t be necessary if all the dependencies are in the same transaction.) |
| Comment by Manu B [ 05/Jul/18 ] |
|
Do we need to take up removal of blocking calls in the current activity? It could have impact in the functionalities. There are 5 usage of ElanUtils::waitForTransactionToComplete.(blocking calls) Please let me know your views. |
| Comment by Stephen Kitt [ 05/Jul/18 ] |
|
As always, if it makes the patch unwieldy, it’s fine to split the patch up. Especially if work is being done in other patches to address these issues, it makes sense to push an initial patch converting what can easily be converted, and then follow up with another patch once the other patches have been merged. (But this bug shouldn’t be marked as “done” until everything is done.) Note that converting code to use a single transaction which is passed down avoids a number of these issues: changes made in a transaction are visible inside that transaction, which means some waits can be removed. There should also be fewer race conditions as a result of this work. |
| Comment by Michael Vorburger [ 25/Jul/18 ] |
|
with https://git.opendaylight.org/gerrit/#/c/74026/ not yet, let's re-use this issue, for clear status. |