[improve][broker] PIP-192: Added VersionId in ServiceUnitStateData #19620
Conversation
Demogorgon314
changed the title
| } | ||
|
|
||
| public static ServiceUnitState state(ServiceUnitStateData data) { | ||
| return data == null ? ServiceUnitState.Init : data.state(); | ||
| } | ||
|
|
||
| public static long versionId(ServiceUnitStateData data) { | ||
| return data == null ? 0 : data.versionId(); |
There was a problem hiding this comment.
Can we use the VERSION_ID_INIT as the default value?
There was a problem hiding this comment.
No. VERSION_ID_INIT is 1, and for the null data, it needs to be zero in order to make the first non-null version be 1( 0 + 1).
We can return VERSION_ID_INIT - 1(which is zero) if null.
There was a problem hiding this comment.
We can do that too. Then, we don't need to call versionId(data) since we already did the null check.
private long getNextVersionId(String serviceUnit) {
var data = tableview.get(serviceUnit);
return getNextVersionId(data);
}
private long getNextVersionId(ServiceUnitStateData data) {
return data == null ? VERSION_ID_INIT : data.getVersionId() + 1;
}
Updated the code.
Nevertheless, if this is the first version(child bundle creation), we can pass the VERSION_ID_INIT without calling getNextVersionId(null). It looks redundant to call getNextVersionId(null) to get VERSION_ID_INIT.
ServiceUnitStateData next = new ServiceUnitStateData(Owned, data.broker(), VERSION_ID_INIT);
or
ServiceUnitStateData next = new ServiceUnitStateData(Owned, data.broker(), getNextVersionId(null));
| } | ||
|
|
||
| public static ServiceUnitState state(ServiceUnitStateData data) { | ||
| return data == null ? ServiceUnitState.Init : data.state(); | ||
| } | ||
|
|
||
| public static long versionId(ServiceUnitStateData data) { | ||
| return data == null ? 0 : data.versionId(); |
| if (from != null && from.versionId() + 1 != to.versionId()) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Do we need to filter out the invalidate data when reading from the topic? Otherwise, we might get an inconsistent data view with the compacted data.
There was a problem hiding this comment.
Yes, when reading msgs from the table views or strategic compaction, we do apply this same strategy to filter out invalid states.
There was a problem hiding this comment.
Ah, got it. The table view also apply this change.
heesung-sn
force-pushed
the
pip-192-bsc-versionid
branch
from
4bdec08 to
715139d
Compare
| if (from != null && from.versionId() + 1 != to.versionId()) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Ah, got it. The table view also apply this change.
Technoboy-
added
ready-to-test
type/feature
Master Issue: #16691
Motivation
There is a possible edge case where bundle ownership could be negatively impacted by delayed commands in the new load balancer.
Let's say a (network-partitioned) broker has a delayed
ServiceUnitStateChanneland triggers a transfer or unload command based on it. This transfer or unload command should be rejected as it is from an outdated context.To reject this outdated transition, I propose
versionIdinServiceUnitStateData, which monotonically increases for its subsequent state changes until tombstoned.ServiceUnitStateCompactionStrategycan reject messages with older versions than the previous ones(assert prev+1 == cur).After this PR,
... Owned(versionId=50) -> Transfer_Assigned(versionId=45) // invalid
... Owned(versionId=50) -> Transfer_Assigned(versionId=51) // valid
Modifications
This PR
versionIdinServiceUnitStateDataVerifying this change
This change added tests and can be verified as follows:
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
docdoc-requireddoc-not-neededdoc-completeWe will have separate PRs to update the Doc later.
Matching PR in forked repository
PR in forked repository: heesung-sn#33