[Network Config Mgr] Support on-demand service and GS persistence service #607
Conversation
Codecov Report
@@ Coverage Diff @@
## master #607 +/- ##
============================================
- Coverage 32.65% 32.01% -0.65%
- Complexity 1248 1255 +7
============================================
Files 522 525 +3
Lines 13156 13439 +283
Branches 1610 1657 +47
============================================
+ Hits 4296 4302 +6
- Misses 8275 8556 +281
+ Partials 585 581 -4
Continue to review full report at Codecov.
|
|
This pull request fixes 4 alerts when merging e748285 into 0777179 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 4 alerts when merging 3ad0cf6 into 0777179 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 4 alerts when merging c7e9849 into 0777179 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 4 alerts when merging ddac42a into 0777179 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 4 alerts when merging 937c3f0 into 0777179 - view on LGTM.com fixed alerts:
|
|
This pull request introduces 1 alert and fixes 4 when merging cc6edaf into a78e1d4 - view on LGTM.com new alerts:
fixed alerts:
|
|
This pull request introduces 1 alert and fixes 4 when merging 7f67ae2 into a78e1d4 - view on LGTM.com new alerts:
fixed alerts:
|
| public class VpcResourceCache { | ||
|
|
||
| // Map <VNI, Map<PIP, List<ResoruceIDType>> | ||
| // Map <VNI, Map<PIP, ResourceMetadata> |
There was a problem hiding this comment.
TXN:
Need a "CacheFactory cacheFactory" member which will be assigned the cacheFactory param passed into the Constructor.
The following method is also needed:
public Transaction getTransaction() {
return cacheFactory.getTransaction();
}
The same change is needed in HostResourceMetadataCache and, ResourceCacheManager classes.
Futhermore, the following method is needed in lib/src/main/java/com/futurewei/alcor/common/db/CacheFactory.java
public Transaction getTransaction() {
return iCacheFactory.getTransaction();
}
| } catch (Exception e) { | ||
| e.printStackTrace(); | ||
| } | ||
| } |
There was a problem hiding this comment.
TXN:
This entire loop can be pushed into goalStatePersistenceService.updateGoalState() and the call it self invoked after grpcGoalStateClient.sendGoalStates(filteredGoalStates); in the try{} block below.
The abouve loop inside goalStatePersistenceService.updateGoalState() will have to be modified as follows:
goalStatePersistenceService.updateGoalState() {
try {
Transaction txn = getTransaction();
txn.start();
txn.commit();
} cartch () {}
This way, if the send fails we don't have to do any rollbacks.
I have added
public Transaction getTransaction() {
if(hrmCache != null)
return hrmCache.getTransaction();
if (rssCache != null)
return rssCache.getTransaction();
if (vpcCache != null)
return vpcCache.getTransaction();
return null;
}
In my equivalent of service/impl/GoalStatePersistenceServiceImpl.java.
There is the issue of capturing changes from updates to ACA into NCM caches, which, I don't think is handled by this code. I might be wrong, in which case a clear comment to that effect will help a lot.
There was a problem hiding this comment.
Sounds good. As discussed, we will merge the current NCM implementation for non-transactional perf test. Meanwhile please work on the other PR on Transaction Mode changes (PR #604) and get it merged.
Next step is continue refactoring this part to enable transactional support and do a quick perf test.
|
This pull request fixes 4 alerts when merging 8dcfda2 into a78e1d4 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 4 alerts when merging 565e102 into a78e1d4 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 4 alerts when merging 83a5a1e into a78e1d4 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 4 alerts when merging 06d3d25 into a78e1d4 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 4 alerts when merging 37d579e into a78e1d4 - view on LGTM.com fixed alerts:
|
xieus
changed the title
|
This pull request fixes 4 alerts when merging e7352ab into a78e1d4 - view on LGTM.com fixed alerts:
|
| // 4. Bingo! this packet is allowed, collect port related resources (NEIGHBOR, SG etc. FULL GS) and send down | ||
| // to ACA by a separate gRPC client |
|
This pull request fixes 4 alerts when merging 179979f into 1639de4 - view on LGTM.com fixed alerts:
|
4 participants
This PR adds the following features:
Note that this PR passes integration test with Test Controller (PR #560) and ACA.