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.

Sign up for GitHub

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

Jump to bottom

[core] Make the refreshAsync options take affect to PrimaryKeyPartialLookupTable #4208

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[core] Make the refreshAsync options take affect to PrimaryKeyPartialLookupTable #4208

wants to merge 1 commit into from

Conversation

xiangyuf
Copy link
Contributor

@xiangyuf xiangyuf commented Sep 18, 2024 •
edited
Loading

Purpose

Linked issue: close #3386

Support async refresh for PrimaryKeyPartialLookupTable

Tests

  1. Updated existing test LookupTableTest#testPKLookupTableRefreshAsync for refresh async
  2. Added test LookupTableTest#testPartialLookupTableRefreshAsync for PrimaryKeyPartialLookupTable

API and Format

Documentation

@xiangyuf xiangyuf changed the title [core] Support async refresh for PrimaryKeyPartialLookupTable [WIP][core] Support async refresh for PrimaryKeyPartialLookupTable Sep 18, 2024
@xiangyuf xiangyuf force-pushed the 3386 branch 2 times, most recently from 1f69190 to a623f81 Compare September 18, 2024 15:42
@xiangyuf xiangyuf changed the title [WIP][core] Support async refresh for PrimaryKeyPartialLookupTable [core] Support async refresh for PrimaryKeyPartialLookupTable Sep 18, 2024
@xiangyuf
Copy link
Contributor Author

@FangYongs @JingsongLi Would you kindly review this when you have time?

@xiangyuf
Copy link
Contributor Author

@JingsongLi
Copy link
Contributor

CC @liming30 too

@liming30
Copy link
Contributor

The purpose of introducing asynchronous refresh in #3297 is to solve the problem that local data refresh will block lookup. Is this issue also trying to solve the same problem? PrimaryKeyPartialLookupTable.refresh only refreshes metadata, and it seems that it cannot solve the problem of synchronous refresh of local data.

@xiangyuf
Copy link
Contributor Author

The purpose of introducing asynchronous refresh in #3297 is to solve the problem that local data refresh will block lookup. Is this issue also trying to solve the same problem? PrimaryKeyPartialLookupTable.refresh only refreshes metadata, and it seems that it cannot solve the problem of synchronous refresh of local data.

@liming30 Thx for reply. I see your point. Maybe I should split this task into two tasks: 1, Make the refreshAsync options also work for PrimaryKeyPartialLookupTable; 2, Improve the refresh behavior for LocalQueryExecutor, create local LookupFile during refresh according to the before and after DataFileMetas in a DataSplit. WDYT?

@xiangyuf
Copy link
Contributor Author

xiangyuf commented Oct 8, 2024

Hi @liming30 @Aitozi, after second thought I think AsyncRefreshLookupTable should not handle the consistence issue between asyncRefresh operation and get operation for the time being. This can be resolved inside PrimaryKeyPartialLookupTable and FullCacheLookupTable. WDYT?

@xiangyuf xiangyuf changed the title [core] Support async refresh for PrimaryKeyPartialLookupTable [core] Make the refreshAsync options take affect to PrimaryKeyPartialLookupTable Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liming30 liming30 liming30 left review comments

@Aitozi Aitozi Aitozi left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature] Support async refresh for PrimaryKeyPartialLookupTable
4 participants
@xiangyuf @JingsongLi @liming30 @Aitozi