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

Recreate TabletsMetadata iterator when file ranges are not contiguous #5341

Open
wants to merge 5 commits into
base: 2.1
Choose a base branch
from
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -325,9 +325,8 @@ private long loadFiles(TableId tableId, Path bulkDir, LoadMappingIterator loadMa

Text startRow = loadMapEntry.getKey().prevEndRow();

Iterator<TabletMetadata> tabletIter =
TabletsMetadata.builder(manager.getContext()).forTable(tableId).overlapping(startRow, null)
.checkConsistency().fetch(PREV_ROW, LOCATION, LOADED).build().iterator();
TabletsMetadata tm = TabletsMetadata.builder(manager.getContext()).forTable(tableId)
.overlapping(startRow, null).checkConsistency().fetch(PREV_ROW, LOCATION, LOADED).build();

Loader loader;
if (bulkInfo.tableState == TableState.ONLINE) {
Expand All @@ -339,11 +338,21 @@ private long loadFiles(TableId tableId, Path bulkDir, LoadMappingIterator loadMa
loader.start(bulkDir, manager, tid, bulkInfo.setTime);

long t1 = System.currentTimeMillis();
KeyExtent prevLastExtent = null; // KeyExtent of last tablet from prior loadMapEntry
while (lmi.hasNext()) {
loadMapEntry = lmi.next();
List<TabletMetadata> tablets = findOverlappingTablets(loadMapEntry.getKey(), tabletIter);
KeyExtent loadMapKey = loadMapEntry.getKey();
if (prevLastExtent != null && !loadMapKey.isPreviousExtent(prevLastExtent)) {
Copy link
Contributor

@keith-turner keith-turner Feb 25, 2025

Choose a reason for hiding this comment

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

In some case this strategy could potentially make performance worse, like the case of importing into every 3rd tablet. The underlying scanner has already made an RPC and fetched some number of key values. Not sure of the best way to do this, but ideally we would only reset the scanner if the needed data is not already sitting in that batch of key values that was already read.

Copy link
Contributor

@keith-turner keith-turner Feb 25, 2025

Choose a reason for hiding this comment

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

Wondering if using a batch scanner would be better here to minimize the overall number of RPCs made. Would be a large change to the code. The current code, even if we optimize the use of the scanner will make a lot of RPCs for some cases (like importing into every 100th tablet in a million tablet table) and those RPCs will be made serially. A batch scanner would minimize the number of RPCs made for these cases. Batch scanner could also make RPCs in parallel. The Scanner is probably more efficient for reading tablets sequentially, but probably not much slower than the batch scanner. I suspect the batch scanner would not be much slower when data is contiguous and would be much better when data is sparse.

Would be good to gather some performance data before making large changes to improve performance to ensure they are needed. Can not do it in 2.1, but in main we could experiment w/ the SplitMillionIT and try doing things like importing into every 10th tablet for 1000 tablets, every 100th tablet for 1000 tablets, etc.

tm.close();
tm = TabletsMetadata.builder(manager.getContext()).forTable(tableId)
.overlapping(loadMapKey.prevEndRow(), null).checkConsistency()
.fetch(PREV_ROW, LOCATION, LOADED).build();
}
List<TabletMetadata> tablets = findOverlappingTablets(loadMapKey, tm.iterator());
loader.load(tablets, loadMapEntry.getValue());
prevLastExtent = tablets.get(tablets.size() - 1).getExtent();
}
tm.close();

long sleepTime = loader.finish();
if (sleepTime > 0) {
Expand Down