Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check if server's and client's versions are compatible #65

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tellet-q
Copy link
Contributor

@tellet-q tellet-q commented Dec 19, 2024

Send request to fetch server version during client init and compare with client version.

Introduce a boolean param SkipCompatibilityCheck (false by default). If it is false -> check versions during client init and generate a warning if versions are not compatible. Compatible versions are the ones that differ by one, i.e client version 1.6.x is compatible with server versions 1.5.x and 1.7.x but not 1.4.x or 1.8.x.

@tellet-q tellet-q requested a review from Anush008 December 19, 2024 12:11
qdrant/grpc_client.go Outdated Show resolved Hide resolved
@tellet-q tellet-q requested a review from Anush008 December 20, 2024 12:51
return &Version{
Major: major,
Minor: minor,
Rest: rest,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest is not used for any compatibility checks.

Comment on lines +73 to +93
func IsCompatible(clientVersion, serverVersion *string) bool {
if *clientVersion == *serverVersion {
return true
}

parsedClientVersion, err := ParseVersion(*clientVersion)
if err != nil {
log.Printf("Unable to compare versions: %v", err)
return false
}
parsedServerVersion, err := ParseVersion(*serverVersion)
if err != nil {
log.Printf("Unable to compare versions: %v", err)
return false
}
majorDiff := int(math.Abs(float64(parsedClientVersion.Major - parsedServerVersion.Major)))
if majorDiff >= 1 {
return false
}
return int(math.Abs(float64(parsedClientVersion.Minor-parsedServerVersion.Minor))) <= 1
}
Copy link
Member

@Anush008 Anush008 Dec 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func IsCompatible(clientVersion, serverVersion *string) bool {
if *clientVersion == *serverVersion {
return true
}
parsedClientVersion, err := ParseVersion(*clientVersion)
if err != nil {
log.Printf("Unable to compare versions: %v", err)
return false
}
parsedServerVersion, err := ParseVersion(*serverVersion)
if err != nil {
log.Printf("Unable to compare versions: %v", err)
return false
}
majorDiff := int(math.Abs(float64(parsedClientVersion.Major - parsedServerVersion.Major)))
if majorDiff >= 1 {
return false
}
return int(math.Abs(float64(parsedClientVersion.Minor-parsedServerVersion.Minor))) <= 1
}
func IsCompatible(clientVersion, serverVersion string) bool {
if clientVersion == serverVersion {
return true
}
client, err := ParseVersion(clientVersion)
if err != nil {
log.Printf("Unable to compare versions: %v", err)
return false
}
server, err := ParseVersion(serverVersion)
if err != nil {
log.Printf("Unable to compare versions: %v", err)
return false
}
if client.Major != server.Major {
return false
}
diff := client.Minor - server.Minor
return diff <= 1 && diff >= -1
}

Could be simpler.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plus. Please use slog WARN.

logger.Warn("Failed to obtain server version. " +
"Unable to check client-server compatibility. " +
"Set SkipCompatibilityCheck=true to skip version check.")
} else if !IsCompatible(&clientVersion, &serverVersion) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
} else if !IsCompatible(&clientVersion, &serverVersion) {
} else if !IsCompatible(clientVersion, serverVersion) {

for _, test := range tests {
clientVersion := test.clientVersion
serverVersion := test.serverVersion
result := qdrant.IsCompatible(&clientVersion, &serverVersion)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
result := qdrant.IsCompatible(&clientVersion, &serverVersion)
result := qdrant.IsCompatible(clientVersion, serverVersion)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants