Uh oh!
There was an error while loading. Please reload this page.
PHOENIX-5998 Paged server side ungrouped aggregate operations - #936
PHOENIX-5998 Paged server side ungrouped aggregate operations#936kadirozde wants to merge 6 commits into
Conversation
stoty
commented
Oct 23, 2020
💔 -1 overall
This message was automatically generated. |
| value)); | ||
| if (result == null) { | ||
| if (hasRows) { | ||
| return null; |
There was a problem hiding this comment.
This doesn't match with the above comment of always returning rows.
There was a problem hiding this comment.
I did not really change the behavior here and thus I did not change the comment. If there is no row in the table, we should still get a result object with the zero row count. This is all what the comment tries to communicate.
stoty
commented
Oct 25, 2020
💔 -1 overall
This message was automatically generated. |
stoty
commented
Oct 26, 2020
💔 -1 overall
This message was automatically generated. |
stoty
commented
Oct 26, 2020
💔 -1 overall
This message was automatically generated. |
kadirozde
commented
Oct 26, 2020
@ChinmaySKulkarni, @gjacoby126, or anyone else who wants to review this PR, if you do not have any questions or comments, can I get your approval? Thanks |
gjacoby126
commented
Oct 26, 2020
@kadirozde - UpsertSelectIT seems to have crashed in the last test run. (And timeouts for SequencePointInTimeIT and IndexToolForNonTxGlobalIndexIT). Have these passed elsewhere? |
kadirozde
commented
Oct 26, 2020
Please check the failures, they are due to minicluster setup and operation timeout. They pass for me locally. |
stoty
commented
Oct 27, 2020
💔 -1 overall
This message was automatically generated. |
@kadirozde The changes are substantial and I will need some heads-down time to review them. If it is urgent, please feel free to rely on other's reviews and don't wait for me. I plan on taking a look in detail within the next couple of days. |
kadirozde
commented
Oct 27, 2020
No, it is not urgent. I was going to start working on PHOENIX-6207 and there is some dependency between them and so I wanted to push this before starting the other. But it is okay and I do not have to wait for this PR to be checked in. Please take your time. Yes, the changes are substantial but mostly mechanic. The core of the change is that instead of scanning the entire table region in the postScannerOpen hook and returning the result of the aggregate operation for the entire table region in one result iteration, this PR just returns a region scanner (i.e., an new scanner called UngroupedAggregateRegionScanner) in the postScannerOpen hook for the UngroupedAggregateRegionObserver coproc, and then applies the aggregate operation on a chunk (i.e, page) of a table region in each result iteration. This means the client needs to do many iterations in order to process a table region and aggregate the results of these pages on the client side. Please note that previously, the client needed to aggregate the results of server side aggregations, one for each table region (not for each table region page). Hope this helps. |
There was a problem hiding this comment.
@kadirozde overall looks like a great improvement. I have added a few comments. Some questions:
Is it more beneficial to have paging based on row size rather than number of rows, since each row can be arbitrarily large?
Server-side pagination will help reduce the chance of the race conditions mentioned in the Jira description, but does not aim at eliminating them, correct?
Though this is aimed at such race conditions related to mutations (server-side UPSERT SELECT/DELETE), it seems like it will also affect the normal read path for non-Group_By aggregate queries. Is there any negative effect/extra slowness during reads due to this pagination, and if yes, do we want to make sure that changes only affect the write paths?
Let's also please add some tests for this.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
kadirozde
commented
Nov 10, 2020
|
ChinmaySKulkarni
left a comment
There was a problem hiding this comment.
LGTM. Thanks for addressing my comments @kadirozde. Can you please open a follow-up documentation Jira to add some info about this useful feature and also the new config to enable it. Thanks
kadirozde
commented
Nov 10, 2020
@ChinmaySKulkarni, Thank you for reviewing this and asking very good questions. I just had a conversation on this with Sukumar Maddineni and we agreed that the limit on the paging size should be in terms of the time to spend on the server side instead of the number of rows or bytes. If you agree too, I would like to update the PR accordingly. Also, I will write a doc that will cover this and the other two Jiras (6207 and 6211). |
ChinmaySKulkarni
commented
Nov 10, 2020
No problem, thanks for the quick turnaround. I think that makes sense too. It will be interesting to see how to handle corner cases where for example, we have processed the last row partially. A design doc will be very helpful. |
stoty
commented
Nov 10, 2020
💔 -1 overall
This message was automatically generated. |
Uh oh!
There was an error while loading. Please reload this page.
| conf.setInt(NUM_CONCURRENT_INDEX_WRITER_THREADS_CONF_KEY, 1); | ||
| conf.setInt(GLOBAL_INDEX_ROW_AGE_THRESHOLD_TO_DELETE_MS_ATTRIB, 0); | ||
| if (conf.getLong(QueryServices.UNGROUPED_AGGREGATE_PAGE_SIZE_IN_MS, 0) == 0) { | ||
| conf.setLong(QueryServices.UNGROUPED_AGGREGATE_PAGE_SIZE_IN_MS, 0); |
There was a problem hiding this comment.
Did you mean to set this to some non-zero value?
There was a problem hiding this comment.
I set it to 1 ms first but observed that even within 1ms pages, several iterations can be processed in a page . So, in order to make sure that every test is subject to paging, I had to set it to 0, which results in one-row pages.
There was a problem hiding this comment.
I meant, the if condition is checking that the config is 0 and then setting it to zero itself. Perhaps you meant to invert the condition
There was a problem hiding this comment.
Oh nevermind. I see that you're passing in 0 as the default in the getLong.
Uh oh!
There was an error while loading. Please reload this page.
stoty
commented
Nov 11, 2020
💔 -1 overall
This message was automatically generated. |
stoty
commented
Nov 12, 2020
💔 -1 overall
This message was automatically generated. |
gjacoby126
left a comment
There was a problem hiding this comment.
Exciting change, thanks for this @kadirozde . I have mostly nits.
| result.setKeyValues(results); | ||
| } | ||
| void deleteRow(List<Cell> results, UngroupedAggregateRegionObserver.MutationList mutations) { | ||
| // FIXME: the version of the Delete constructor without the lock |
There was a problem hiding this comment.
This comment isn't relevant anymore. (The RowLock constructor for deletes is long gone)
There was a problem hiding this comment.
I will remove the comment
| } else if (isDelete) { | ||
| deleteRow(results, mutations); | ||
| } else if (isUpsert) { | ||
| upsert(result, ptr, mutations); |
There was a problem hiding this comment.
Thank you SO much for these Extract Methods. :-)
| List<IndexMaintainer> indexMaintainers; | ||
| boolean isPKChanging = false; | ||
| long ts; | ||
| PhoenixTransactionProvider txnProvider = null; |
There was a problem hiding this comment.
Lots of package-level variables -- can any of these be made private? If an external class is referring to them (such as UARO), that's too-tight coupling. But it looks like these are mostly transplanted locals from UARO, so they should be safe to make private?
There was a problem hiding this comment.
I will make all of them private as you suggested.
| } | ||
| public void commitBatchWithRetries(final Region region, final List<Mutation> localRegionMutations, final long blockingMemstoreSize) throws IOException { | ||
| Configuration getUpsertSelectConfig() { |
There was a problem hiding this comment.
Package scope is intentional in these methods?
There was a problem hiding this comment.
Yes. Initially these were class private. I made them public unnecessarily in one of my previous Jiras when I introduced IndexRebuildRegionScanner. Based on Chinmay's feedback, I made them package private as they are only accessed within the package.
| byte[] oldRow = Bytes.copy(firstKV.getRowArray(), firstKV.getRowOffset(), firstKV.getRowLength()); | ||
| for (Cell cell : results) { | ||
| // Copy existing cell but with new row key | ||
| Cell newCell = new KeyValue(newRow, 0, newRow.length, |
There was a problem hiding this comment.
KeyValue is IA.Private -- I know this is extracted from existing code, but good to get rid of these when we find them. Can be replaced with CellUtil.createCell and CellUtil.cloneFamily, etc.
| case Put: | ||
| // If Put, point delete old Put | ||
| Delete del = new Delete(oldRow); | ||
| del.addDeleteMarker(new KeyValue(cell.getRowArray(), cell.getRowOffset(), cell.getRowLength(), |
| + region.getRegionInfo().getRegionNameAsString(), e); | ||
| throw e; | ||
| } | ||
| KeyValue keyValue; |
There was a problem hiding this comment.
This can also be a Cell populated by CellUtil below at 632.
stoty
commented
Nov 13, 2020
💔 -1 overall
This message was automatically generated. |
No description provided.