Skip to content

Commit

Permalink
code review changes
Browse files Browse the repository at this point in the history
  • Loading branch information
@XingY
XingY committed Oct 5, 2024
1 parent a66ceae commit 17b5926
Show file tree
Hide file tree
Showing 4 changed files with 57 additions and 60 deletions.
61 changes: 33 additions & 28 deletions experiment/src/org/labkey/experiment/ExpDataIterators.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2050,9 +2050,9 @@ private static String dataRole(ExpData data, User user)
public static class SearchIndexIteratorBuilder implements DataIteratorBuilder
{
final DataIteratorBuilder _pre;
final Function<Pair<List<String>, List<Integer>>, Runnable> _indexFunction;
final Function<SearchIndexDataKeys, Runnable> _indexFunction;

public SearchIndexIteratorBuilder(DataIteratorBuilder pre, Function<Pair<List<String>, List<Integer>>, Runnable> indexFunction)
public SearchIndexIteratorBuilder(DataIteratorBuilder pre, Function<SearchIndexDataKeys, Runnable> indexFunction)
{
_pre = pre;
_indexFunction = indexFunction;
Expand All @@ -2066,17 +2066,19 @@ public DataIterator getDataIterator(DataIteratorContext context)
}
}

public record SearchIndexDataKeys(@NotNull List<Integer> orderedRowIds, @NotNull List<String> lsids) { }

private static class SearchIndexIterator extends WrapperDataIterator
{
final DataIteratorContext _context;
final Integer _lsidCol;
final Integer _rowIdCol;
final ArrayList<String> _lsids;
final ArrayList<Integer> _rowIds;
final Function<Pair<List<String>, List<Integer>>, Runnable> _indexFunction;
final boolean _useExistingRecord;
final Function<SearchIndexDataKeys, Runnable> _indexFunction;
final boolean _isInsert;

protected SearchIndexIterator(DataIterator di, DataIteratorContext context, Function<Pair<List<String>, List<Integer>>, Runnable> indexFunction)
protected SearchIndexIterator(DataIterator di, DataIteratorContext context, Function<SearchIndexDataKeys, Runnable> indexFunction)
{
super(di);
_context = context;
Expand All @@ -2089,7 +2091,7 @@ protected SearchIndexIterator(DataIterator di, DataIteratorContext context, Func
_lsids = new ArrayList<>(100);
_rowIds = new ArrayList<>(100);

_useExistingRecord = context.getInsertOption().updateOnly && _lsidCol == null;
_isInsert = !context.getInsertOption().allowUpdate; // only useRowIdCol for INSERT. For UPDATE, rowId usually is not available. For MERGE, rowId is a new DBSequence value for existing data
}

@Override
Expand All @@ -2099,32 +2101,34 @@ public boolean next() throws BatchValidationException

if (hasNext)
{
if (_useExistingRecord)
Integer rowId = null;
String lsid = null;
if (_isInsert)
{
rowId = _rowIdCol == null ? null : (Integer) get(_rowIdCol);
if (rowId == null)
lsid = _lsidCol == null ? null : (String) get(_lsidCol);
}
else
{
Map<String, Object> map = getExistingRecord();
if (map != null)
{
if (map.containsKey("rowId")) // favor rowId over lsid to avoid additional query for indexing
{
Integer rowIdObj = (Integer) map.get("rowId");
if (null != rowIdObj)
_rowIds.add(rowIdObj);
}
else if (map.containsKey("lsid"))
{
String lsid = (String) map.get("lsid");
if (null != lsid)
_lsids.add(lsid);
}
rowId = (Integer) map.get("rowId");
if (rowId == null && map.containsKey("lsid"))
lsid = (String) map.get("lsid");
}
}
else
{
// don't use rowId from data since it's backed by DBSequence and might not be the same as the actual rowId
String lsid = _lsidCol == null ? null : (String) get(_lsidCol);
if (null != lsid)
_lsids.add(lsid);

// for UPDATE/MERGE, don't use _rowIdCol
if (rowId == null && lsid == null)
lsid = _lsidCol == null ? null : (String) get(_lsidCol);
}

if (rowId != null)
_rowIds.add(rowId);
if (lsid != null)
_lsids.add(lsid);
}
else
{
Expand All @@ -2133,7 +2137,8 @@ else if (map.containsKey("lsid"))
{
final ArrayList<String> lsids = new ArrayList<>(_lsids);
final ArrayList<Integer> rowIds = new ArrayList<>(_rowIds);
final Runnable indexTask = _indexFunction.apply(new Pair<>(lsids, rowIds));
Collections.sort(rowIds);
final Runnable indexTask = _indexFunction.apply(new SearchIndexDataKeys(rowIds, lsids));

if (null != DbScope.getLabKeyScope())
DbScope.getLabKeyScope().addCommitTask(indexTask, DbScope.CommitTaskOption.POSTCOMMIT);
Expand Down Expand Up @@ -2241,7 +2246,7 @@ public static class PersistDataIteratorBuilder implements DataIteratorBuilder
private final Set<String> _excludedColumns = new HashSet<>(List.of("generated","runId","sourceapplicationid")); // generated has database DEFAULT 0

private String _fileLinkDirectory = null;
Function<Pair<List<String>, List<Integer>>, Runnable> _indexFunction;
Function<SearchIndexDataKeys, Runnable> _indexFunction;
final Map<String, String> _importAliases;

// expTable is the shared experiment table e.g. exp.Data or exp.Materials
Expand All @@ -2256,7 +2261,7 @@ public PersistDataIteratorBuilder(@NotNull DataIteratorBuilder in, TableInfo exp
_importAliases = importAliases != null ? new CaseInsensitiveHashMap<>(importAliases) : new CaseInsensitiveHashMap<>();
}

public PersistDataIteratorBuilder setIndexFunction(Function<Pair<List<String>, List<Integer>>, Runnable> indexFunction)
public PersistDataIteratorBuilder setIndexFunction(Function<SearchIndexDataKeys, Runnable> indexFunction)
{
_indexFunction = indexFunction;
return this;
Expand Down
19 changes: 13 additions & 6 deletions experiment/src/org/labkey/experiment/api/ExpDataClassDataTableImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -922,13 +922,9 @@ public DataIteratorBuilder persistRows(DataIteratorBuilder data, DataIteratorCon
if (null != searchService)
{
final var scope = propertiesTable.getSchema().getScope();
step0.setIndexFunction(lsidRowIds -> scope.addCommitTask(() ->
step0.setIndexFunction(searchIndexDataKeys -> scope.addCommitTask(() ->
{
List<String> lsids = lsidRowIds.first;
List<Integer> orderedRowIds = new ArrayList<>(lsidRowIds.second);
if (orderedRowIds.isEmpty() && !lsids.isEmpty()) // either lsids or rowIds is empty
orderedRowIds = experimentServiceImpl.getOrderedRowIdsFromLsids(experimentServiceImpl.getTinfoData(), lsids);

List<Integer> orderedRowIds = searchIndexDataKeys.orderedRowIds();
// Issue 51263: order by RowId to reduce deadlock
ListUtils.partition(orderedRowIds, 100).forEach(sublist ->
searchService.defaultTask().addRunnable(SearchService.PRIORITY.group, () ->
Expand All @@ -938,6 +934,17 @@ public DataIteratorBuilder persistRows(DataIteratorBuilder data, DataIteratorCon
return (Void) null;
}))
);

List<String> lsids = searchIndexDataKeys.lsids();
ListUtils.partition(lsids, 100).forEach(sublist ->
searchService.defaultTask().addRunnable(SearchService.PRIORITY.group, () ->
scope.executeWithRetryReadOnly(tx -> {
for (ExpDataImpl expData : experimentServiceImpl.getExpDatasByLSID(sublist))
expData.index(searchService.defaultTask(), this);
return (Void) null;
}))
);

}, DbScope.CommitTaskOption.POSTCOMMIT));
}
DataIteratorBuilder builder = LoggingDataIterator.wrap(step0);
Expand Down
16 changes: 11 additions & 5 deletions experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1546,12 +1546,10 @@ public DataIteratorBuilder persistRows(DataIteratorBuilder data, DataIteratorCon

if (null != searchService)
{
persist.setIndexFunction(lsidRowIds -> propertiesTable.getSchema().getScope().addCommitTask(() ->
persist.setIndexFunction(searchIndexDataKeys -> propertiesTable.getSchema().getScope().addCommitTask(() ->
{
List<String> lsids = lsidRowIds.first;
List<Integer> orderedRowIds = new ArrayList<>(lsidRowIds.second);
if (orderedRowIds.isEmpty() && !lsids.isEmpty()) // either lsids or rowIds is empty
orderedRowIds = experimentServiceImpl.getOrderedRowIdsFromLsids(experimentServiceImpl.getTinfoMaterial(), lsids);
List<String> lsids = searchIndexDataKeys.lsids();
List<Integer> orderedRowIds = searchIndexDataKeys.orderedRowIds();

// Issue 51263: order by RowId to reduce deadlock
ListUtils.partition(orderedRowIds, 100).forEach(sublist ->
Expand All @@ -1561,6 +1559,14 @@ public DataIteratorBuilder persistRows(DataIteratorBuilder data, DataIteratorCon
expMaterial.index(searchService.defaultTask());
})
);

ListUtils.partition(lsids, 100).forEach(sublist ->
searchService.defaultTask().addRunnable(SearchService.PRIORITY.group, () ->
{
for (ExpMaterialImpl expMaterial : experimentServiceImpl.getExpMaterialsByLsid(sublist))
expMaterial.index(searchService.defaultTask());
})
);
}, DbScope.CommitTaskOption.POSTCOMMIT)
);
}
Expand Down
21 changes: 0 additions & 21 deletions experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -687,27 +687,6 @@ public ExpRun createRunForProvenanceRecording(Container container, User user, Re
return getExpDatas(new SimpleFilter(FieldKey.fromParts(ExpDataTable.Column.LSID.name()), lsids, IN));
}


public List<Integer> getOrderedRowIdsFromLsids(TableInfo tableInfo, List<String> lsids)
{
List<Integer> allRowIds = new ArrayList<>();
ListUtils.partition(lsids, 100).forEach(sublist ->
{
List<Integer> rowIds = new TableSelector(
tableInfo,
Collections.singleton("RowId"),
new SimpleFilter(FieldKey.fromParts("LSID"), lsids, CompareType.IN),
null
).getArrayList(Integer.class);
allRowIds.addAll(rowIds);
}
);

Collections.sort(allRowIds);

return allRowIds;
}

@Override
public @NotNull List<ExpDataImpl> getExpDatas(Collection<Integer> rowIds)
{
Expand Down

0 comments on commit 17b5926

Please sign in to comment.