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

Support preferable location for connections #121

Merged

Conversation

a1kaigorodov
Copy link
Contributor

No description provided.

@@ -117,6 +118,10 @@ private static GrpcTransportBuilder makeGrpcTransportBuilder(@NonNull YdbConfig
throw new IllegalArgumentException("one of [discoveryEndpoint, hostAndPort] must be set");
}

if (config.getPreferableLocation() != null) {
transportBuilder.withBalancingSettings(BalancingSettings.fromLocation(config.getPreferableLocation()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see that YDB Java SDK has more BalancingSettings than we add in this PR.

At the very least, it also has the "detect data center automatically" setting BalancingSettings.detectLocalDs() (with Ds instead of Dc in the name, which seems like a misspelling).

There also is the (presumably, default) BalancingSettings.defaultInstance() that means "use all nodes for client balancing".

Let's maybe add a dedicated BalancingConfig structure to YdbConfig to reflect these options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds reasonable. I revised the PR

@a1kaigorodov a1kaigorodov force-pushed the support-preferable-location branch from 264d712 to f207450 Compare February 6, 2025 07:42
Copy link
Collaborator

@nvamelichev nvamelichev left a comment

Choose a reason for hiding this comment

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

@a1kaigorodov
Copy link
Contributor Author

It looks like there are unused imports in YqlOrderBy.java in the main branch.
Should I fix it in this PR?

@nvamelichev
Copy link
Collaborator

@a1kaigorodov Yes, please fix the unused imports in YqlOrderBy in this PR 🤗

@a1kaigorodov a1kaigorodov force-pushed the support-preferable-location branch from f207450 to 36ee8bc Compare February 7, 2025 15:04
@a1kaigorodov
Copy link
Contributor Author

Done

@nvamelichev nvamelichev merged commit e35a85c into ydb-platform:main Feb 8, 2025
1 check passed
@a1kaigorodov
Copy link
Contributor Author

@nvamelichev Can you please let me know when the next release is scheduled?

@nvamelichev
Copy link
Collaborator

nvamelichev commented Feb 10, 2025

We'll probably make a release early this week.

Note that the release will also include:

  • Cleanup of naming logic for nested fields of Entities. Legacy logic was kept as default for YOJ 2.x, but you should explicitly use @Column(columnNaming=...) to choose the desired naming behavior.
  • Cleanup changes in YqlStatementPart and its implementations (YqlLimit, YqlOrderBy). Custom YqlStatements might be potentially affected by these changes, but in practice everything should be fine ;-)

Please indicate if you need a release of the older 2.5.x with this PR cherry-picked, or if you've already migrated to 2.6.x.

@a1kaigorodov
Copy link
Contributor Author

We still use the older version. It would be great if you could backport this pull request for 2.5.

@nvamelichev
Copy link
Collaborator

This PR is now in release 2.6.6 (already in Maven Central).
2.5.x release will follow a little later today.

@nvamelichev
Copy link
Collaborator

✅ Also fixed in backport release 2.5.13 (already on Maven Central).

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