ibessonov commented on code in PR #1800:
URL: https://github.com/apache/ignite-3/pull/1800#discussion_r1147358153
##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexBuilder.java:
##########
@@ -111,13 +112,13 @@ private class BuildIndexTask implements Runnable {
private final int partitionId;
- private final boolean firstBatch;
+ private final @Nullable RowId nextFirstRowId;
- private BuildIndexTask(TableImpl table, TableIndexView tableIndexView,
int partitionId, boolean firstBatch) {
+ private BuildIndexTask(TableImpl table, TableIndexView tableIndexView,
int partitionId, @Nullable RowId nextFirstRowId) {
this.table = table;
this.tableIndexView = tableIndexView;
this.partitionId = partitionId;
- this.firstBatch = firstBatch;
+ this.nextFirstRowId = nextFirstRowId;
Review Comment:
I actually believe that you should do that the way I recommend, because the
code in "run" is very messed up.
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/index/hash/PageMemoryHashIndexStorage.java:
##########
@@ -91,12 +91,15 @@ public class PageMemoryHashIndexStorage implements
HashIndexStorage {
* @param freeList Free list to store index columns.
* @param hashIndexTree Hash index tree instance.
* @param indexMetaTree Index meta tree instance.
+ * @param lastBuiltIndexRowUuid Last row ID uuid that has been processed
by an ongoing index build process, {@code null} if the process
+ * has finished.
*/
public PageMemoryHashIndexStorage(
HashIndexDescriptor descriptor,
IndexColumnsFreeList freeList,
HashIndexTree hashIndexTree,
- IndexMetaTree indexMetaTree
+ IndexMetaTree indexMetaTree,
+ @Nullable UUID lastBuiltIndexRowUuid
Review Comment:
I recommended passing meta object, not the id itself, can you please change
that?
Meta is an object, that has everything grouped, and passing such object into
a constructor is a good thing. Passing some arbitrary uuid and, potentially,
the rest of meta fields, looks wrong
##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexBuilder.java:
##########
@@ -111,13 +112,13 @@ private class BuildIndexTask implements Runnable {
private final int partitionId;
- private final boolean firstBatch;
+ private final @Nullable RowId nextFirstRowId;
- private BuildIndexTask(TableImpl table, TableIndexView tableIndexView,
int partitionId, boolean firstBatch) {
+ private BuildIndexTask(TableImpl table, TableIndexView tableIndexView,
int partitionId, @Nullable RowId nextFirstRowId) {
this.table = table;
this.tableIndexView = tableIndexView;
this.partitionId = partitionId;
- this.firstBatch = firstBatch;
+ this.nextFirstRowId = nextFirstRowId;
Review Comment:
`null` value here is very confusing. What do you really pass into this
constructor? The "lastBuiltRowId()" from the index? You could do the same thing
on the initial creation, you know?
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/RowId.java:
##########
@@ -163,4 +163,11 @@ public int compareTo(RowId rowId) {
public String toString() {
return "RowId [partitionId=" + partitionId() + ", uuid=" + uuid + ']';
}
+
+ /**
+ * Returns {@code true} if the lowest row ID.
+ */
+ public boolean isLowest() {
Review Comment:
I don't like the fact that this method exists. First of all, I think that
it's a sign of badly designed algorithms, when it comes to usages. Second, this
method could have been easily implemented outside of the RowId class. Why do
you pollute interface with such unnecessary things, if you already have
"equals"?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
