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

GH-5234: fix limited programmatic configuration support of FedX source selection cache #5235

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

aschwarte10
Copy link
Contributor

@aschwarte10 aschwarte10 commented Jan 20, 2025

GitHub issue resolved: #5234

Add an additional constructor accepting a supplier for an initialized cache.


PR Author Checklist (see the contributor guidelines for more details):

  • my pull request is self-contained
  • I've added tests for the changes I made
  • I've applied code formatting (you can use mvn process-resources to format from the command line)
  • I've squashed my commits where necessary
  • every commit message starts with the issue number (GH-xxxx) followed by a meaningful description of the change

@aschwarte10 aschwarte10 added the 📦 fedx fedx: optimized federated query support label Jan 20, 2025
@aschwarte10 aschwarte10 changed the title GH-5234: fix limited support of FedX source selection cache GH-5234: fix limited configuration support of FedX source selection cache Jan 20, 2025
Add an additional constructor accepting a supplier for an initialized
cache.
@aschwarte10 aschwarte10 force-pushed the GH-5234-source-selection-cache branch from 9b19efc to a22d277 Compare January 20, 2025 11:00
@aschwarte10 aschwarte10 changed the title GH-5234: fix limited configuration support of FedX source selection cache GH-5234: fix limited programmatic configuration support of FedX source selection cache Jan 20, 2025
Copy link

@jetztgradnet jetztgradnet left a comment

Choose a reason for hiding this comment

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

LGTM. An additional constructor should not cause any binary incompatibilities.

@hmottestad
Copy link
Contributor

This is a new feature though and not really a bug fix.

@aschwarte10
Copy link
Contributor Author

This is a new feature though and not really a bug fix.

Technically, you are absolutely right, good eyes. Let me try to explain why I reported this as bug, which in our case impacts performance / stability of a federated system. For our use-cases more control over the cache strategies (eviction, size, introspection) is required to offer good performance.

If this change is not acceptable for a patch release, we in our case would (temporarily) need to override/customize some parts of the source selection logic to make the guava cache accessible. Please let me know.

@hmottestad
Copy link
Contributor

We can change it from bug to performance and then we can include it.

@aschwarte10
Copy link
Contributor Author

We can change it from bug to performance and then we can include it.

Thanks, I see that you already did this. Good to know!

@aschwarte10 aschwarte10 merged commit 230a437 into main Jan 23, 2025
9 checks passed
@aschwarte10 aschwarte10 deleted the GH-5234-source-selection-cache branch January 23, 2025 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 fedx fedx: optimized federated query support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FedX: Limited support to influence source selection cache parameters
3 participants