Fix message upgrade and config downgrades#864
Conversation
grafnu
left a comment
grafnu
left a comment
There was a problem hiding this comment.
Just one minor real comment and then a few nits and pontifications
| VERSION_1_5_0("1.5.0", 105000), | ||
| VERSION_1_4_2("1.4.2", 104020), | ||
| VERSION_1_4_0("1.4.0", 104000); | ||
| VERSION_1_4_1("1.4.1", 104010), |
There was a problem hiding this comment.
These don't seem right, I would expect 1_4_1 to be 010401 (not 104010) and 1.3.14 to be 010314 (not 103014)... ? Something doesn't line up right!
There was a problem hiding this comment.
You're right - I just followed the existing numbering. They all need changing
| final Integer targetVersion = targetVersionEnum.value(); | ||
|
|
||
| // downgrade to 1.4.1 | ||
| if (currentVersion() > VERSION_1_4_1.value() && currentVersion() > targetVersion){ |
There was a problem hiding this comment.
If we wanted to be really smarmy here we could make a table of sorts that had { versionA, versionB, downgradeFunc, upgradeFun } in it and then all this if statement stuff would be automagic!
Not something I'd suggest for now, but just something to keep in mind for the rainy day in the future when you're bored.
| message.put(VERSION_KEY, String.format(TARGET_FORMAT, major, minor, patch)); | ||
| } else { | ||
| // Files are now updated to Current version of UDMI | ||
| if(message.has(VERSION_KEY)){ |
There was a problem hiding this comment.
lint won't like this? Should have space after "if"
Also, comment is unclear or superfluous -- it's not particularly clarifying the why of anything?
There was a problem hiding this comment.
Comment updated - Even if the message was not modified, it is now conformant to the current version of UDMI, so update the version property if it exists
Maybe worth a quick thought on if this is correct?
The main reason it was added was because the udmi_site_model devices have a future metadata version of 1.5.1. Consequently, trying to "downgrade" the config to 1.5.1 caused errors because the version it didn't exist. My solution was to correct the version at source. It also made sense because at the end of the upgrade process, even if unmodified, the message schema is "current". Whereas presently, we can have version "1" of files coming through.
I guess by stomping over the version, we lose the original information? Is it a problem? I could leave it as is and solve my unrecognized version differently.
| private void upgradeTo_1_5_0_metadata() { | ||
|
|
||
| } | ||
| private void upgradeTo_1_4_1_metadata() { |
|
There should be another field in the "upgraded_from" to capture the
needful. I agree with the logic that the end result should be consistent
(on current version) even if the source was not changed.
***@***.***:~/udmi$ fgrep upgraded_from -r schema/
schema/events_pointset.json: "upgraded_from": {
schema/metadata.json: "upgraded_from": {
schema/state_pointset.json: "upgraded_from": {
schema/state.json: "upgraded_from": {
schema/state_system.json: "upgraded_from": {
schema/events_system.json: "upgraded_from": {
…On Thu, Apr 25, 2024 at 5:21 AM Noureddine ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In common/src/main/java/com/google/udmi/util/MessageUpgrader.java
<#864 (comment)>:
> upgraded |= !before.equals(message);
patch = 1;
}
if (upgraded && message.get(VERSION_KEY) != null) {
message.put(UPGRADED_FROM, originalVersion);
message.put(VERSION_KEY, String.format(TARGET_FORMAT, major, minor, patch));
+ } else {
+ // Files are now updated to Current version of UDMI
+ if(message.has(VERSION_KEY)){
Comment updated - Even if the message was not modified, it is now
conformant to the current version of UDMI, so update the version property
if it exists
Maybe worth a quick thought on if this is correct?
The main reason it was added was because the udmi_site_model devices have
a future metadata version of 1.5.1. Consequently, I was trying to
"downgrade" the config to "1.5.1" which caused errors because it didn't
exist. My solution was to correct the version at source. It also made sense
because at the end of the upgrade process, even if unmodified, the message
schema is "current". Whereas presently, we can have version "1" of files
coming through.
I guess by stomping over the version, we lose the original information? Is
it a problem? I could leave it as is and solve my unrecognized version
differently.
—
Reply to this email directly, view it on GitHub
<#864 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIEPD3DJTMPY4C37K24YRLY7DDJHAVCNFSM6AAAAABGO6TU4WVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMRRHE4TCNJYHA>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
2 participants
No description provided.