Rename InstanceName to SiloName. #1740
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gabikliot
force-pushed
the
InstanceName-Rename
branch
from
8b053ba
to
d0c4085
Compare
|
@shayhatsor should express his opinion about ZK backward compat. |
| @@ -46,7 +46,7 @@ public override void WriteJson(JsonWriter writer, object value, JsonSerializer s | |||
| { | |||
| SiloAddress = jo["SiloAddress"].ToObject<SiloAddress>(serializer), | |||
| HostName = jo["HostName"].ToObject<string>(), | |||
| InstanceName = jo["InstanceName"].ToObject<string>(), | |||
| SiloName = jo["SiloName"].ToObject<string>(), | |||
There was a problem hiding this comment.
This will throw when SiloName doesn't exist. I'll need to improve serialization here to account for this in an elegant way. In the meantime, it can just be
SiloName = (jo["SiloName"]??jo["InstanceName"]).ToObject<string>(),
|
@gabikliot, about Consul membership. It doesn't populate the |
gabikliot
force-pushed
the
InstanceName-Rename
branch
from
d0c4085
to
95ff2a2
Compare
|
Addressed both comments: ZK and Consul. |
|
There were some strange, likely unrelated, failures when I ran functional tests twice. Rerunning again to be sure. |
|
The test failures still seem unrelated. So I'll merge this. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses #1738.
One important issue to watch is backward compatibility: since InstanceName is persisted, what will happen if we suddenly rename it to SiloName? Our clients should work without experiencing any pain, for example without needing to fully stop the cluster and delete the whole table.
I addressed backward compatibility issue on Azure. I kept both columns in the table. New silo will write its SiloName into both columns, and when reading entries of other silos it will try to parse SiloName from either of the columns.
I believe this will allow a mixed cluster (with old and new version) to co-exist. However, I have not tested that. In general, in other systems, this is a rather complicated and delicate issue that has to be dealt with great caution and usually solved strategically (as opposite to my "one-off" solution here).
As for SQL and ZK and Consul - I have not addressed backward compatibility there.
SQL does not currently have InstanceName at all, so it should not be a problem.
Consul does not populate InstanceName at all, so it should not be a problem.
ZK is the only one that needs to be dealt with.