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

feat: Add TableDataService#getRawTableLocationProvider #5966

Merged
merged 20 commits into from
Sep 5, 2024

Conversation

arman-ddl
Copy link
Contributor

@arman-ddl arman-ddl commented Aug 22, 2024

Satisfies #6001.

Copy link
Member

@rcaudy rcaudy left a comment

Choose a reason for hiding this comment

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

Trivial review. This appear to be in the spirit of what we discussed, I just think we should be more precise in the documentation. Darin's comments re: correctness seem valid.

Originally, I thought looking up a raw TLP should not apply filters, but I'm convinced you've made the right call; the TLP is raw, but which TLP we select still applies the TDS's rules.

arman-ddl and others added 3 commits August 29, 2024 07:33
Per Darin and Ryan

Co-authored-by: Ryan Caudy <rcaudy@gmail.com>
Co-authored-by: Darin Petty <github@fourpettys.com>
@arman-ddl arman-ddl added this to the 0.37.0 milestone Aug 29, 2024
@arman-ddl arman-ddl marked this pull request as ready for review August 29, 2024 16:38
rcaudy
rcaudy previously approved these changes Aug 30, 2024
abaranec
abaranec previously approved these changes Aug 30, 2024
Per Darin

Co-authored-by: Darin Petty <github@fourpettys.com>
@arman-ddl arman-ddl dismissed stale reviews from abaranec and rcaudy via 3677e8e September 3, 2024 18:17
Copy link
Contributor

@darinpetty darinpetty left a comment

Choose a reason for hiding this comment

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

With the two comments below, it looks like you could end up with multiple services that cause a false collision in CompositeTDS. If CTDS has (directly or indirectly) two FilteredTDSs that could potentially handle the TLK, both will be returned and you will get an error, even if you have crafted a system where they don't actually collide, as in the LTDS scenario I described in slack.
I think the fix is to query each candidate TLP with hasTableLocation to find out if it actually has a location, not just might have it.

@arman-ddl
Copy link
Contributor Author

With the two comments below, it looks like you could end up with multiple services that cause a false collision in CompositeTDS. If CTDS has (directly or indirectly) two FilteredTDSs that could potentially handle the TLK, both will be returned and you will get an error, even if you have crafted a system where they don't actually collide, as in the LTDS scenario I described in slack. I think the fix is to query each candidate TLP with hasTableLocation to find out if it actually has a location, not just might have it.

Darin's observations makes me think about a couple more things:

  1. Should we also be TDS#reset'ing if the TLP does not hasTableLocationKey since the TLP we create is not actually going to be used?
  2. Going even further, should the TDS interface instead have a hasTableLocationKey method to accompany getRawTableLocationProvider so we don't have to do an unnecessary TLP creation and/or reset?

@rcaudy
Copy link
Member

rcaudy commented Sep 4, 2024

With the two comments below, it looks like you could end up with multiple services that cause a false collision in CompositeTDS. If CTDS has (directly or indirectly) two FilteredTDSs that could potentially handle the TLK, both will be returned and you will get an error, even if you have crafted a system where they don't actually collide, as in the LTDS scenario I described in slack. I think the fix is to query each candidate TLP with hasTableLocation to find out if it actually has a location, not just might have it.

Darin's observations makes me think about a couple more things:

  1. Should we also be TDS#reset'ing if the TLP does not hasTableLocationKey since the TLP we create is not actually going to be used?
  2. Going even further, should the TDS interface instead have a hasTableLocationKey method to accompany getRawTableLocationProvider so we don't have to do an unnecessary TLP creation and/or reset?
  1. Reset is too heavy of a hammer. Realistically, if you're doing this, how likely do you think it is that the TLP doesn't already exist?
  2. I don't known if you need that.

Maybe you could take Darin's suggestion and go one step further: if the TLP doesn't exist at some level, or no TLP is found that has the TLK in question, return null from getRawTableLocationProvider.

@arman-ddl
Copy link
Contributor Author

arman-ddl commented Sep 4, 2024

With the two comments below, it looks like you could end up with multiple services that cause a false collision in CompositeTDS. If CTDS has (directly or indirectly) two FilteredTDSs that could potentially handle the TLK, both will be returned and you will get an error, even if you have crafted a system where they don't actually collide, as in the LTDS scenario I described in slack. I think the fix is to query each candidate TLP with hasTableLocation to find out if it actually has a location, not just might have it.

Darin's observations makes me think about a couple more things:

  1. Should we also be TDS#reset'ing if the TLP does not hasTableLocationKey since the TLP we create is not actually going to be used?
  2. Going even further, should the TDS interface instead have a hasTableLocationKey method to accompany getRawTableLocationProvider so we don't have to do an unnecessary TLP creation and/or reset?
  1. Reset is too heavy of a hammer. Realistically, if you're doing this, how likely do you think it is that the TLP doesn't already exist?
  2. I don't known if you need that.

Maybe you could take Darin's suggestion and go one step further: if the TLP doesn't exist at some level, or no TLP is found that has the TLK in question, return null from getRawTableLocationProvider.

I'm thinking of a couple ways to determine "if the TLP doesn't exist at some level":

  1. Adding hasTableLocationProvider method to TDS or ATDS
  2. Changing AbstractTableDataService#tableLocationProviders from private to protected

Are you fine w/ either? If not, how do you propose going about it?

@arman-ddl
Copy link
Contributor Author

Off-GH, Ryan helped me realize the issues associated w/ the default TDS#getRawTlp impl. Removing it and adding an ATDS#getRawTlp impl seems to address both Darin's and my concerns.

rcaudy
rcaudy previously approved these changes Sep 4, 2024
Copy link
Member

@rcaudy rcaudy left a comment

Choose a reason for hiding this comment

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

LGTM

@arman-ddl arman-ddl marked this pull request as draft September 5, 2024 15:43
@arman-ddl arman-ddl marked this pull request as ready for review September 5, 2024 16:37
@arman-ddl arman-ddl merged commit b748158 into deephaven:main Sep 5, 2024
16 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants