Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2053163002:
[Offline pages] Adding persistent request queue based on SQLite (Closed)
|
Created:
4 years, 6 months ago by fgorski Modified:
3 years, 8 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Offline pages] Adding persistent request queue based on SQLite
* This patch adds request queue SQLite base store.
* Converst in memory store tests to typed tests and adapts them to SQLite.
BUG=607221
Committed: https://crrev.com/5ff2a7b64f096594f1970eb18c61996972268835
Cr-Commit-Position: refs/heads/master@{#401622}
Patch Set 1 #Patch Set 2 : Refactoring request queue store tests and adding tests for SQL store #Patch Set 3 : Fixing the code to pass all the tests #Patch Set 4 : Rebasing #
Total comments: 18
Patch Set 5 : Adding DEPS #Patch Set 6 : Fixing gyp and gn files #Patch Set 7 : Fixing save page request #Patch Set 8 : Addressing feedback from Pete #Patch Set 9 : Updating the gypi file to include sqlite based store #
Total comments: 6
Patch Set 10 : Updating documentation and moving code around per Doug's review #
Total comments: 8
Patch Set 11 : Addressing code review feedback from petewil #
Total comments: 54
Patch Set 12 : Addressing CR feedback from shess@ #Patch Set 13 : Rebasing (after renaming test file outside) #Patch Set 14 : Updating the data definition code and getting rid of the enum to access columns #
Total comments: 7
Patch Set 15 : Fixing code comments #
Total comments: 2
Messages
Total messages: 34 (8 generated)
fgorski@chromium.org changed reviewers: + dougarnett@chromium.org, petewil@chromium.org
Please, start looking.
Description was changed from ========== [Offline pages] Adding persistent request queue based on SQLite * This patch adds request queue SQLite base store. * (still to come) Adds tests for the store. BUG=607221 ========== to ========== [Offline pages] Adding persistent request queue based on SQLite * This patch adds request queue SQLite base store. * Converst in memory store tests to typed tests and adapts them to SQLite. BUG=607221 ==========
Comments for everything except the unittest for SQL (that will come later) https://codereview.chromium.org/2053163002/diff/60001/components/components_t... File components/components_tests.gyp (right): https://codereview.chromium.org/2053163002/diff/60001/components/components_t... components/components_tests.gyp:435: 'offline_pages/background/request_queue_store_unittest.cc', Should we keep the in memory store unittest? The memory store sure is handy for other unit tests. https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... File components/offline_pages/background/request_queue_in_memory_store.cc (right): https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... components/offline_pages/background/request_queue_in_memory_store.cc:46: bool result = true; If result is never false, do we still need it in the callback? If so, should we get rid of the result variable and just pass true in the callback for the memory queue? https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... File components/offline_pages/background/request_queue_in_memory_store_unittest.cc (left): https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... components/offline_pages/background/request_queue_in_memory_store_unittest.cc:5: #include "components/offline_pages/background/request_queue_in_memory_store.h" Maybe we should keep this in addition to the sql unittest. https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... File components/offline_pages/background/request_queue_store_sql.cc (right): https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... components/offline_pages/background/request_queue_store_sql.cc:103: SavePageRequest MakeSavePageRequest(sql::Statement* statement) { Note to self: What is this statement object? It isn't just text for the query? This appears to wrap the returned row. https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... components/offline_pages/background/request_queue_store_sql.cc:103: SavePageRequest MakeSavePageRequest(sql::Statement* statement) { Should this return a pointer so we don't keep copying the SavePageRequest? https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... components/offline_pages/background/request_queue_store_sql.cc:149: // Count and add/update as part of a single transaction. Grammar nit: Did you mean "Count an" instead of "Count and"? https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... components/offline_pages/background/request_queue_store_sql.cc:189: db->set_page_size(4096); Maybe these numbers (and string) should be constants in the .h file, or at the very least at the top of this file in the anonymous namespace. https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... components/offline_pages/background/request_queue_store_sql.cc:307: void RequestQueueStoreSQL::ResetSync( What does this delete, everything? A comment would be nice. https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... components/offline_pages/background/request_queue_store_sql.cc:316: void RequestQueueStoreSQL::GetRequests(const GetRequestsCallback& callback) { The next three methods seem awfully similar - is there an opportunity for refactoring here? (If so, a TODO is fine).
PTAL Pete, the tests for in memory store are moving to a typed test file. We are still testing it. https://codereview.chromium.org/2053163002/diff/60001/components/components_t... File components/components_tests.gyp (right): https://codereview.chromium.org/2053163002/diff/60001/components/components_t... components/components_tests.gyp:435: 'offline_pages/background/request_queue_store_unittest.cc', On 2016/06/14 21:54:45, Pete Williamson wrote: > Should we keep the in memory store unittest? The memory store sure is handy for > other unit tests. we are, we are converting the test to a typed test to make sure both stores work the same way. https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... File components/offline_pages/background/request_queue_in_memory_store.cc (right): https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... components/offline_pages/background/request_queue_in_memory_store.cc:46: bool result = true; On 2016/06/14 21:54:45, Pete Williamson wrote: > If result is never false, do we still need it in the callback? > If so, should we get rid of the result variable and just pass true in the > callback for the memory queue? done. https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... File components/offline_pages/background/request_queue_in_memory_store_unittest.cc (left): https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... components/offline_pages/background/request_queue_in_memory_store_unittest.cc:5: #include "components/offline_pages/background/request_queue_in_memory_store.h" On 2016/06/14 21:54:45, Pete Williamson wrote: > Maybe we should keep this in addition to the sql unittest. we are. https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... File components/offline_pages/background/request_queue_store_sql.cc (right): https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... components/offline_pages/background/request_queue_store_sql.cc:103: SavePageRequest MakeSavePageRequest(sql::Statement* statement) { On 2016/06/14 21:54:45, Pete Williamson wrote: > Note to self: What is this statement object? It isn't just text for the query? > This appears to wrap the returned row. It defines a statement before execution and acts as a cursor after. https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... components/offline_pages/background/request_queue_store_sql.cc:103: SavePageRequest MakeSavePageRequest(sql::Statement* statement) { On 2016/06/14 21:54:47, Pete Williamson wrote: > Should this return a pointer so we don't keep copying the SavePageRequest? The object is not that big. It is ok to copy for now at least. An alternative is to deal with memory allocation and freeing, which might turn more expensive. https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... components/offline_pages/background/request_queue_store_sql.cc:149: // Count and add/update as part of a single transaction. On 2016/06/14 21:54:47, Pete Williamson wrote: > Grammar nit: Did you mean "Count an" instead of "Count and"? I meant it how it was, but rephrased so that it is easier to understand. Let me know if that works. https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... components/offline_pages/background/request_queue_store_sql.cc:189: db->set_page_size(4096); On 2016/06/14 21:54:45, Pete Williamson wrote: > Maybe these numbers (and string) should be constants in the .h file, or at the > very least at the top of this file in the anonymous namespace. I looked through the source code. With only a few exceptions this is exactly how things are done. The way I read it is: name of the setter documents the meaning of the value that follows and we are never reusing it in the file, so verbatim const is ok. https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... components/offline_pages/background/request_queue_store_sql.cc:307: void RequestQueueStoreSQL::ResetSync( On 2016/06/14 21:54:46, Pete Williamson wrote: > What does this delete, everything? A comment would be nice. Done. https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... components/offline_pages/background/request_queue_store_sql.cc:316: void RequestQueueStoreSQL::GetRequests(const GetRequestsCallback& callback) { On 2016/06/14 21:54:45, Pete Williamson wrote: > The next three methods seem awfully similar - is there an opportunity for > refactoring here? (If so, a TODO is fine). They follow the same structure: check for db issue appropriate result when no db post to background thread to accomplish a task. If you look at callback types and parameters of the methods we post to, you'll see that only the structure is similar. I don't think this warrants refactoring.
On 2016/06/14 22:38:05, fgorski wrote: > PTAL > > Pete, the tests for in memory store are moving to a typed test file. We are > still testing it. > > https://codereview.chromium.org/2053163002/diff/60001/components/components_t... > File components/components_tests.gyp (right): > > https://codereview.chromium.org/2053163002/diff/60001/components/components_t... > components/components_tests.gyp:435: > 'offline_pages/background/request_queue_store_unittest.cc', > On 2016/06/14 21:54:45, Pete Williamson wrote: > > Should we keep the in memory store unittest? The memory store sure is handy > for > > other unit tests. > > we are, we are converting the test to a typed test to make sure both stores work > the same way. > > https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... > File components/offline_pages/background/request_queue_in_memory_store.cc > (right): > > https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... > components/offline_pages/background/request_queue_in_memory_store.cc:46: bool > result = true; > On 2016/06/14 21:54:45, Pete Williamson wrote: > > If result is never false, do we still need it in the callback? > > If so, should we get rid of the result variable and just pass true in the > > callback for the memory queue? > > done. > > https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... > File > components/offline_pages/background/request_queue_in_memory_store_unittest.cc > (left): > > https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... > components/offline_pages/background/request_queue_in_memory_store_unittest.cc:5: > #include "components/offline_pages/background/request_queue_in_memory_store.h" > On 2016/06/14 21:54:45, Pete Williamson wrote: > > Maybe we should keep this in addition to the sql unittest. > > we are. > > https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... > File components/offline_pages/background/request_queue_store_sql.cc (right): > > https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... > components/offline_pages/background/request_queue_store_sql.cc:103: > SavePageRequest MakeSavePageRequest(sql::Statement* statement) { > On 2016/06/14 21:54:45, Pete Williamson wrote: > > Note to self: What is this statement object? It isn't just text for the > query? > > This appears to wrap the returned row. > > It defines a statement before execution and acts as a cursor after. > > https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... > components/offline_pages/background/request_queue_store_sql.cc:103: > SavePageRequest MakeSavePageRequest(sql::Statement* statement) { > On 2016/06/14 21:54:47, Pete Williamson wrote: > > Should this return a pointer so we don't keep copying the SavePageRequest? > > The object is not that big. It is ok to copy for now at least. An alternative is > to deal with memory allocation and freeing, which might turn more expensive. > > https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... > components/offline_pages/background/request_queue_store_sql.cc:149: // Count and > add/update as part of a single transaction. > On 2016/06/14 21:54:47, Pete Williamson wrote: > > Grammar nit: Did you mean "Count an" instead of "Count and"? > > I meant it how it was, but rephrased so that it is easier to understand. Let me > know if that works. > > https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... > components/offline_pages/background/request_queue_store_sql.cc:189: > db->set_page_size(4096); > On 2016/06/14 21:54:45, Pete Williamson wrote: > > Maybe these numbers (and string) should be constants in the .h file, or at the > > very least at the top of this file in the anonymous namespace. > > I looked through the source code. With only a few exceptions this is exactly how > things are done. The way I read it is: name of the setter documents the meaning > of the value that follows and we are never reusing it in the file, so verbatim > const is ok. > > https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... > components/offline_pages/background/request_queue_store_sql.cc:307: void > RequestQueueStoreSQL::ResetSync( > On 2016/06/14 21:54:46, Pete Williamson wrote: > > What does this delete, everything? A comment would be nice. > > Done. > > https://codereview.chromium.org/2053163002/diff/60001/components/offline_page... > components/offline_pages/background/request_queue_store_sql.cc:316: void > RequestQueueStoreSQL::GetRequests(const GetRequestsCallback& callback) { > On 2016/06/14 21:54:45, Pete Williamson wrote: > > The next three methods seem awfully similar - is there an opportunity for > > refactoring here? (If so, a TODO is fine). > > They follow the same structure: > check for db > issue appropriate result when no db > post to background thread to accomplish a task. > > If you look at callback types and parameters of the methods we post to, you'll > see that only the structure is similar. I don't think this warrants refactoring. All changes look good. I'll look at the unit test file tomorrow.
fgorski@chromium.org changed reviewers: + shess@chromium.org
+shess@ for extensive review of SQLite implementation of the store, please.
lgtm https://codereview.chromium.org/2053163002/diff/160001/components/offline_pag... File components/offline_pages/background/request_queue_store_unittest.cc (right): https://codereview.chromium.org/2053163002/diff/160001/components/offline_pag... components/offline_pages/background/request_queue_store_unittest.cc:34: bool operator==(const SavePageRequest& lhs, const SavePageRequest& rhs) { Is this a common pattern to define in test rather than in class itself (to save prod code size)? I wonder if we end up wanting this in some test utils helper if wanted in multiple places. https://codereview.chromium.org/2053163002/diff/160001/components/offline_pag... components/offline_pages/background/request_queue_store_unittest.cc:67: class RequestQueueStoreTestBase : public testing::Test { could have doc here about test suite of request queue interface run across both impls https://codereview.chromium.org/2053163002/diff/160001/components/offline_pag... components/offline_pages/background/request_queue_store_unittest.cc:171: TYPED_TEST_CASE(RequestQueueStoreTest, StoreTypes); So this will run tests for both store types? Might be good to add comment for poor naive maintainers.
Addressed https://codereview.chromium.org/2053163002/diff/160001/components/offline_pag... File components/offline_pages/background/request_queue_store_unittest.cc (right): https://codereview.chromium.org/2053163002/diff/160001/components/offline_pag... components/offline_pages/background/request_queue_store_unittest.cc:34: bool operator==(const SavePageRequest& lhs, const SavePageRequest& rhs) { On 2016/06/15 16:35:46, dougarnett wrote: > Is this a common pattern to define in test rather than in class itself (to save > prod code size)? > I wonder if we end up wanting this in some test utils helper if wanted in > multiple places. So far this is the only place we need it, hence we implement it here. Also, this is old code, but I moved it to another file. https://codereview.chromium.org/2053163002/diff/160001/components/offline_pag... components/offline_pages/background/request_queue_store_unittest.cc:67: class RequestQueueStoreTestBase : public testing::Test { On 2016/06/15 16:35:46, dougarnett wrote: > could have doc here about test suite of request queue interface run across both > impls Done. Good point. Also, in order to make the code a bit more understandable I reordered it a bit. Thanks for suggestion. https://codereview.chromium.org/2053163002/diff/160001/components/offline_pag... components/offline_pages/background/request_queue_store_unittest.cc:171: TYPED_TEST_CASE(RequestQueueStoreTest, StoreTypes); On 2016/06/15 16:35:46, dougarnett wrote: > So this will run tests for both store types? > Might be good to add comment for poor naive maintainers. Done.
https://codereview.chromium.org/2053163002/diff/180001/components/offline_pag... File components/offline_pages/background/request_queue_store_unittest.cc (right): https://codereview.chromium.org/2053163002/diff/180001/components/offline_pag... components/offline_pages/background/request_queue_store_unittest.cc:183: TYPED_TEST_CASE(RequestQueueStoreTest, StoreTypes); Nifty! I like seeing the code only once for both store types! https://codereview.chromium.org/2053163002/diff/180001/components/offline_pag... components/offline_pages/background/request_queue_store_unittest.cc:203: ASSERT_EQ(UpdateStatus::FAILED, this->last_update_status()); Nit - I'm not sure this assert (and others like it) add much, since they are asserting the test infrastructure is correct, not the code under test. Having said that, I don't have any serious objections if you want to keep them. https://codereview.chromium.org/2053163002/diff/180001/components/offline_pag... components/offline_pages/background/request_queue_store_unittest.cc:272: We could also do a Get and make sure the request is not returned, if you wanted more validation. https://codereview.chromium.org/2053163002/diff/180001/components/offline_pag... components/offline_pages/background/request_queue_store_unittest.cc:299: ASSERT_EQ(0ul, this->last_requests().size()); Should we do a get here to verify that the store actually changed?
Pete, PTAL. https://codereview.chromium.org/2053163002/diff/180001/components/offline_pag... File components/offline_pages/background/request_queue_store_unittest.cc (right): https://codereview.chromium.org/2053163002/diff/180001/components/offline_pag... components/offline_pages/background/request_queue_store_unittest.cc:183: TYPED_TEST_CASE(RequestQueueStoreTest, StoreTypes); On 2016/06/15 23:37:05, Pete Williamson wrote: > Nifty! I like seeing the code only once for both store types! Acknowledged. https://codereview.chromium.org/2053163002/diff/180001/components/offline_pag... components/offline_pages/background/request_queue_store_unittest.cc:203: ASSERT_EQ(UpdateStatus::FAILED, this->last_update_status()); On 2016/06/15 23:37:05, Pete Williamson wrote: > Nit - I'm not sure this assert (and others like it) add much, since they are > asserting the test infrastructure is correct, not the code under test. Having > said that, I don't have any serious objections if you want to keep them. Because we use the UpdateStatus enum verbatim, we have to initialize it to some value. This makes sure that before and after pumping the loop the result is different. (As in if someone accidentally changes how the test value is initialized and then the method stops working, we still catch it in this test) https://codereview.chromium.org/2053163002/diff/180001/components/offline_pag... components/offline_pages/background/request_queue_store_unittest.cc:272: On 2016/06/15 23:37:05, Pete Williamson wrote: > We could also do a Get and make sure the request is not returned, if you wanted > more validation. Good point. Done https://codereview.chromium.org/2053163002/diff/180001/components/offline_pag... components/offline_pages/background/request_queue_store_unittest.cc:299: ASSERT_EQ(0ul, this->last_requests().size()); On 2016/06/15 23:37:05, Pete Williamson wrote: > Should we do a get here to verify that the store actually changed? Done. I actually update the reset/recovery function to make it work. Good catch. Thanks.
lgtm
Looks like I did some reviewing, then forgot to send. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... File components/offline_pages/background/request_queue_store_sql.cc (right): https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:72: return db->Execute(sql.c_str()); I think I mentioned that this approach is weird in a past CL. I don't have the energy to figure out how that played out - but I'll just repeat myself, this approach is weird. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:83: if (!CreateTable(db, kRequestQueueTable)) "CREATE TABLE IF NOT EXISTS " should work the same, here. Are you planning to re-use CreateTable() somewhere else? Right now you have CreateSchema() which only appears to exist to wrap CreateTable() in a transaction. It would seem like they could just be merged. [Of course, "CREATE TABLE IF NOT EXISTS " means you wouldn't need a transaction in the first place until you add indices.] https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:92: static const char kSql[] = I don't think you need static here. The literal string is already a static in the TEXT section. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:97: << request_id; // statement.GetSQLStatement(); Drop the logging, or make it VLOG. Or just drop it. My experience is that logging in production code exists solely to confuse build sheriffs. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:103: SavePageRequest MakeSavePageRequest(sql::Statement* statement) { Will const sql::Statement& work, here? My notion is to make it clear that this is read-only WRT statement. AFAICT all of the Column*() methods are const, so it should work. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:105: GURL url(statement->ColumnString(RQ_URL)); Maybe const all of these. consting the GURL, ClientId and base::Time values makes sense as a compiler hint, and once there it makes sense to just const everything to be consistent. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:116: statement->ColumnInt64(RQ_LAST_ATTMEPT_TIME)); Either push last_attempt_time up with the other getters (so that all are in one place), or pull attempt_count below (so that the variables are initialized just before use. Format last_attempt_time consistently with creation_time and activation_time. That said, last_attempt_time and attempt_count seem pretty obvious from context, so having temporaries to store them may not add much, since the enum also describes what you're looking at. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:130: << ", SQL statement: " << statement.GetSQLStatement(); Again, suggest dropping the log line. Note that if the SQL fails because of a syntax error of database corruption, there will already be logging. I believe that for COUNT(*), those are the only failures that could happen (there should _always_ be a single result, otherwise). https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:141: // the same as in the definition/select query. Did I mention this is weird? It's like you're building an object-relational model based entirely on comments. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:153: LOG(ERROR) << "Failed to start transaction on InsertOrReplace."; VLOG or drop the logging. I think the only way transaction creation will fail will be things outside of your control (corruption, etc). I'm not going to comment the rest of the logging, you get the point. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:166: : RequestQueueStore::UpdateStatus::ADDED; Is tracking whether this was added or replaced really necessary? I mean like someone is going to make decisions based on it necessary? You're paying the price of an explicit transaction to accomplish this, if you're doing it just in case someone might use it someday, I'd say just skip the complication and do only what you actually need now. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:196: if (!base::CreateDirectoryAndGetError(path.DirName(), &err)) { Does this need an entire directory for two files (db and journal)? Is there going to be other stuff in here? If there's going to be other stuff in here, should directory creation be a side effect of opening the database, or should the client be making those arrangements? https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:280: return; This boilerplate seems brittle. Would it make sense to have an inner function with bool return and pointer to count, and a wrapper to convert that to the callback? https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:282: int count = 0; I usually expect countable things to be size_t. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:296: } The contents of this loop seem overly complicated. You could just DELETE and test GetLastChangeCount() to see how many rows were affected. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:314: // caution, e.g. when recovering from situation, in which the contents of the "when recovering from situation" doesn't really parse for me. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:317: bool success = base::DeleteFile(db_file_path, /* recursive */ false); Don't use DeleteFile(). Use sql::Connection::Delete(). That said, better would probably be db->Raze(); // Clear all data from database. db->Close(); If you want to also delete after, that's fine :-). Note that there have in the past been cases where delete returning success didn't actually mean delete worked. Many of those cases have been fixed, but I think anything which really depends on the return code from delete is probably broken or going to be broken. Also, AFAICT Reset->ResetSync->OnResetDone->OpenConnection->OpenConnectionSync->InitDatabase->OpenConnectionDone, which means multiple thread hops. Is there any particular reason not to just have ResetSync call InitDatabase and send that success to OnResetDone? https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:367: // to preserve the async style behavior to prevent bugs. Should there be a DCHECK for this case? Also, IMHO this case isn't the same as the !db_.get() case. RemoveRequestsSync cannot operate with a NULL db handle. But it will work just fine with an empty request vector, returning the exact result you're returning below. So I don't think it's worthwhile to short-circuit this case. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:413: LOG(ERROR) << "Database creation fialed."; "fialed", though if the logging were removed entirely that wouldn't be an issue :-). https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:418: // about the success. If you bundled the background processing together as suggested earlier, I don't think you'd need this callback storage. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... File components/offline_pages/background/save_page_request_unittest.cc (right): https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/save_page_request_unittest.cc:93: // Last attempt time, status and attempt count is updated. "are updated"
BTW, I _think_ request_queue_store_unittest.cc is just the deleted file, plus changes. WDYT of landing the rename separately, so that the changes are more reviewable?
Addressed. Please continue reviewing. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... File components/offline_pages/background/request_queue_store_sql.cc (right): https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:72: return db->Execute(sql.c_str()); On 2016/06/20 19:38:32, Scott Hess wrote: > I think I mentioned that this approach is weird in a past CL. I don't have the > energy to figure out how that played out - but I'll just repeat myself, this > approach is weird. Hey, if there is a suggestion how to better write this code I'll gladly look into it, but I didn't find your comment in the previous patch. (looked at like 10 different uploads). Please provide some hints. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:83: if (!CreateTable(db, kRequestQueueTable)) On 2016/06/20 19:38:33, Scott Hess wrote: > "CREATE TABLE IF NOT EXISTS " should work the same, here. > > Are you planning to re-use CreateTable() somewhere else? Right now you have > CreateSchema() which only appears to exist to wrap CreateTable() in a > transaction. It would seem like they could just be merged. > > [Of course, "CREATE TABLE IF NOT EXISTS " means you wouldn't need a transaction > in the first place until you add indices.] Done. I am keeping the CreateTable method separate, as we want to deal with upgrade/indices here. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:92: static const char kSql[] = On 2016/06/20 19:38:32, Scott Hess wrote: > I don't think you need static here. The literal string is already a static in > the TEXT section. Done. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:97: << request_id; // statement.GetSQLStatement(); On 2016/06/20 19:38:32, Scott Hess wrote: > Drop the logging, or make it VLOG. > > Or just drop it. My experience is that logging in production code exists solely > to confuse build sheriffs. Done. This code is a leftover from debugging. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:103: SavePageRequest MakeSavePageRequest(sql::Statement* statement) { On 2016/06/20 19:38:32, Scott Hess wrote: > Will const sql::Statement& work, here? My notion is to make it clear that this > is read-only WRT statement. AFAICT all of the Column*() methods are const, so > it should work. Done. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:105: GURL url(statement->ColumnString(RQ_URL)); On 2016/06/20 19:38:33, Scott Hess wrote: > Maybe const all of these. consting the GURL, ClientId and base::Time values > makes sense as a compiler hint, and once there it makes sense to just const > everything to be consistent. Done. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:116: statement->ColumnInt64(RQ_LAST_ATTMEPT_TIME)); On 2016/06/20 19:38:32, Scott Hess wrote: > Either push last_attempt_time up with the other getters (so that all are in one > place), or pull attempt_count below (so that the variables are initialized just > before use. > > Format last_attempt_time consistently with creation_time and activation_time. Unfortunately that would not work, because they are different length... and git cl format does its own job. > > That said, last_attempt_time and attempt_count seem pretty obvious from context, > so having temporaries to store them may not add much, since the enum also > describes what you're looking at. Done. Went with this approach. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:130: << ", SQL statement: " << statement.GetSQLStatement(); On 2016/06/20 19:38:32, Scott Hess wrote: > Again, suggest dropping the log line. Note that if the SQL fails because of a > syntax error of database corruption, there will already be logging. I believe > that for COUNT(*), those are the only failures that could happen (there should > _always_ be a single result, otherwise). Done. Again debugging -> I had hard time with getting the Count(*) query to work and only after a while figured out that it should be using GetUniqueStatement with Step as opposed to Run... https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:141: // the same as in the definition/select query. On 2016/06/20 19:38:32, Scott Hess wrote: > Did I mention this is weird? It's like you're building an object-relational > model based entirely on comments. OK. I'll gladly remove the comment. Is there a suggestion I should apply to the code? Should I simply use numbers instead of enums? https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:153: LOG(ERROR) << "Failed to start transaction on InsertOrReplace."; On 2016/06/20 19:38:33, Scott Hess wrote: > VLOG or drop the logging. I think the only way transaction creation will fail > will be things outside of your control (corruption, etc). > > I'm not going to comment the rest of the logging, you get the point. Done. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:166: : RequestQueueStore::UpdateStatus::ADDED; On 2016/06/20 19:38:33, Scott Hess wrote: > Is tracking whether this was added or replaced really necessary? I mean like > someone is going to make decisions based on it necessary? You're paying the > price of an explicit transaction to accomplish this, if you're doing it just in > case someone might use it someday, I'd say just skip the complication and do > only what you actually need now. Yeah, I don't like it, but that was requested by another developer. I can try persuasion... (As explained earlier, I figured out that count(*) works differently, so at least I got that going for me...) https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:196: if (!base::CreateDirectoryAndGetError(path.DirName(), &err)) { On 2016/06/20 19:38:33, Scott Hess wrote: > Does this need an entire directory for two files (db and journal)? Is there > going to be other stuff in here? If there's going to be other stuff in here, > should directory creation be a side effect of opening the database, or should > the client be making those arrangements? We like things separated and don't mind having a directory here. Perhaps we'll use it in future to put more stuff in, but for now it is just these 2 files. (It makes it easy to blow away the store when debugging/testing) https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:280: return; On 2016/06/20 19:38:33, Scott Hess wrote: > This boilerplate seems brittle. Would it make sense to have an inner function > with bool return and pointer to count, and a wrapper to convert that to the > callback? Done. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:282: int count = 0; On 2016/06/20 19:38:33, Scott Hess wrote: > I usually expect countable things to be size_t. I played with the idea, however: a) GetLastChangeCount returns int b) our interface is currently set up to expect int in callback. Are you OK with this code sticking to int, at least for now? https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:296: } On 2016/06/20 19:38:33, Scott Hess wrote: > The contents of this loop seem overly complicated. You could just DELETE and > test GetLastChangeCount() to see how many rows were affected. Done. Brilliant idea and it is on the connection not statement, so I can keep DeletebyRequestId the way it is. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:314: // caution, e.g. when recovering from situation, in which the contents of the On 2016/06/20 19:38:32, Scott Hess wrote: > "when recovering from situation" doesn't really parse for me. I am not sure what exactly you didn't parse, but I removed it. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:317: bool success = base::DeleteFile(db_file_path, /* recursive */ false); On 2016/06/20 19:38:33, Scott Hess wrote: > Don't use DeleteFile(). Use sql::Connection::Delete(). > > That said, better would probably be > db->Raze(); // Clear all data from database. > db->Close(); Done. > > If you want to also delete after, that's fine :-). > > Note that there have in the past been cases where delete returning success > didn't actually mean delete worked. Many of those cases have been fixed, but I > think anything which really depends on the return code from delete is probably > broken or going to be broken. OK skipped deleting the file and went with Raze() > > Also, AFAICT > Reset->ResetSync->OnResetDone->OpenConnection->OpenConnectionSync->InitDatabase->OpenConnectionDone, > which means multiple thread hops. Is there any particular reason not to just > have ResetSync call InitDatabase and send that success to OnResetDone? Done. Had to implement it twice. Got to: Reset---(hop to background)-->ResetSync-->InitDatabase---(hop to foreground)-->OnResetDone ---> OpenConnectionDone (called to clean up if we failed). https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:367: // to preserve the async style behavior to prevent bugs. On 2016/06/20 19:38:32, Scott Hess wrote: > Should there be a DCHECK for this case? No that is OK. > Also, IMHO this case isn't the same as the !db_.get() case. RemoveRequestsSync > cannot operate with a NULL db handle. But it will work just fine with an empty > request vector, returning the exact result you're returning below. So I don't > think it's worthwhile to short-circuit this case. Done. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:413: LOG(ERROR) << "Database creation fialed."; On 2016/06/20 19:38:33, Scott Hess wrote: > "fialed", though if the logging were removed entirely that wouldn't be an issue > :-). Done. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:418: // about the success. On 2016/06/20 19:38:33, Scott Hess wrote: > If you bundled the background processing together as suggested earlier, I don't > think you'd need this callback storage. OK. I managed to get rid of reset_callback_. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... File components/offline_pages/background/save_page_request_unittest.cc (right): https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/save_page_request_unittest.cc:93: // Last attempt time, status and attempt count is updated. On 2016/06/20 19:38:33, Scott Hess wrote: > "are updated" Done.
I think the previous cl was https://codereview.chromium.org/1834563002/ by bburns. Is that not related to this? The gist of my concern is that having an enum which must match specific ordering in various queries that are inline text is brittle. There's nothing verifying that later changes maintain your assumptions. You can somewhat address that with good code coverage in tests, but people seldom do that (because it's hard to actually manage it without exposing a bunch of test-only accessors). IMHO for cases like this which look like you're just using one or two sites, it's easier to just inline complete statements and reference things by index and be done with it. That's relatively easier to get decent test coverage for. Another option would be for me to add by-name accessors for bind parameters and columns. That can lead to other convolutions, but maybe that's cleaner than ad-hoc stuff.
On 2016/06/21 00:15:44, Scott Hess wrote: > I think the previous cl was https://codereview.chromium.org/1834563002/ by > bburns. Is that not related to this? Yes, that was another store. I looked at it and at the same area of code, but could not find a relevant comment. > > The gist of my concern is that having an enum which must match specific ordering > in various queries that are inline text is brittle. There's nothing verifying > that later changes maintain your assumptions. You can somewhat address that > with good code coverage in tests, but people seldom do that (because it's hard > to actually manage it without exposing a bunch of test-only accessors). I understand your concern better now and will go ahead and move the SQL statements closer to the place they are used and not try to do the fancy indexing. > > IMHO for cases like this which look like you're just using one or two sites, > it's easier to just inline complete statements and reference things by index and > be done with it. That's relatively easier to get decent test coverage for. > > Another option would be for me to add by-name accessors for bind parameters and > columns. That can lead to other convolutions, but maybe that's cleaner than > ad-hoc stuff.
shess@ ptal I am trying to do my best base on the last message. Let me know where it needs adjusting. Thanks.
On 2016/06/21 20:33:31, fgorski wrote: > shess@ ptal I am trying to do my best base on the last message. Let me know > where it needs adjusting. Apologies, I just came up for air between another code review and heading out for a family event. I'm putting you top-of-list for tomorrow morning, which means ... ping me aggressively if you don't see something by lunch.
I think I'm at LGTM, with only the minor nit about a spelling error. If other commentary causes a change, I'll try to put your review top of queue. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... File components/offline_pages/background/request_queue_store_sql.cc (right): https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:83: if (!CreateTable(db, kRequestQueueTable)) On 2016/06/20 23:43:00, fgorski wrote: > On 2016/06/20 19:38:33, Scott Hess wrote: > > "CREATE TABLE IF NOT EXISTS " should work the same, here. > > > > Are you planning to re-use CreateTable() somewhere else? Right now you have > > CreateSchema() which only appears to exist to wrap CreateTable() in a > > transaction. It would seem like they could just be merged. > > > > [Of course, "CREATE TABLE IF NOT EXISTS " means you wouldn't need a > transaction > > in the first place until you add indices.] > > Done. I am keeping the CreateTable method separate, as we want to deal with > upgrade/indices here. Since you don't have a MetaTable instance or something else tracking version information, do you have a plan for upgrades and indices? I ask because my experience is that if you haven't already done a dry run for a schema upgrade, then sometimes you will find surprising things when you need one. Maybe not in your case, it's just that most of the nasty issues tend to be schema changes due to the combinatorics of the problem. Hmm, and a comment for schema upgrades: I've found that our general system of MetaTable-version-tracking has holes where the code to optimistically create the current version can interact poorly with the ALTER statements. I think it's reasonable to just have schema changes accumulate, so even new users should start with the original schema and upgrade to current. This simplifies the set of paths which can go wrong getting to the final schema, and also makes clear to readers that those intermediate schema may exist in the wild. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:130: << ", SQL statement: " << statement.GetSQLStatement(); On 2016/06/20 23:43:00, fgorski wrote: > On 2016/06/20 19:38:32, Scott Hess wrote: > > Again, suggest dropping the log line. Note that if the SQL fails because of a > > syntax error of database corruption, there will already be logging. I believe > > that for COUNT(*), those are the only failures that could happen (there should > > _always_ be a single result, otherwise). > > Done. Again debugging -> I had hard time with getting the Count(*) query to work > and only after a while figured out that it should be using GetUniqueStatement > with Step as opposed to Run... OK. I've often thought it would be nice to have a "Get a single result" helper, but I've always cut bait because I don't want 6 of them to cover the different types. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:166: : RequestQueueStore::UpdateStatus::ADDED; On 2016/06/20 23:43:00, fgorski wrote: > On 2016/06/20 19:38:33, Scott Hess wrote: > > Is tracking whether this was added or replaced really necessary? I mean like > > someone is going to make decisions based on it necessary? You're paying the > > price of an explicit transaction to accomplish this, if you're doing it just > in > > case someone might use it someday, I'd say just skip the complication and do > > only what you actually need now. > > Yeah, I don't like it, but that was requested by another developer. I can try > persuasion... (As explained earlier, I figured out that count(*) works > differently, so at least I got that going for me...) If someone is going to use it, fine. My main concern is that having the distinction now maybe means someday someone uses it in an incidental fashion, and later you find out that you're writing annoying workaround code to support a use case that nobody ever really intended to support. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:196: if (!base::CreateDirectoryAndGetError(path.DirName(), &err)) { On 2016/06/20 23:43:00, fgorski wrote: > On 2016/06/20 19:38:33, Scott Hess wrote: > > Does this need an entire directory for two files (db and journal)? Is there > > going to be other stuff in here? If there's going to be other stuff in here, > > should directory creation be a side effect of opening the database, or should > > the client be making those arrangements? > > We like things separated and don't mind having a directory here. > Perhaps we'll use it in future to put more stuff in, but for now it is just > these 2 files. > > (It makes it easy to blow away the store when debugging/testing) Acknowledged. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:282: int count = 0; On 2016/06/20 23:42:59, fgorski wrote: > On 2016/06/20 19:38:33, Scott Hess wrote: > > I usually expect countable things to be size_t. > > I played with the idea, however: > a) GetLastChangeCount returns int > b) our interface is currently set up to expect int in callback. > > Are you OK with this code sticking to int, at least for now? OK. I think the generic ruling is something like "use int or size_t". Mostly I'm just used to STL containers using size_t, and this is iterating an STL container. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:317: bool success = base::DeleteFile(db_file_path, /* recursive */ false); On 2016/06/20 23:43:00, fgorski wrote: > On 2016/06/20 19:38:33, Scott Hess wrote: > > Don't use DeleteFile(). Use sql::Connection::Delete(). > > > > That said, better would probably be > > db->Raze(); // Clear all data from database. > > db->Close(); > > Done. > > > > If you want to also delete after, that's fine :-). > > > > Note that there have in the past been cases where delete returning success > > didn't actually mean delete worked. Many of those cases have been fixed, but > I > > think anything which really depends on the return code from delete is probably > > broken or going to be broken. > > OK skipped deleting the file and went with Raze() Given the other point about this class managing a directory containing a database, rather than a database, I guess you maybe end up with a distinction between "Clear the data from the database" and "Delete the database". Raze() is aiming for the first of those, if you later decide to go the delete route, or add a delete function, IMHO delete the directory as a whole, not the files within it. [That said, delete is hairy on Windows. You can have problems with virus scanners and other malware holding handles to files and preventing the delete from working.] https://codereview.chromium.org/2053163002/diff/260001/components/offline_pag... File components/offline_pages/background/DEPS (left): https://codereview.chromium.org/2053163002/diff/260001/components/offline_pag... components/offline_pages/background/DEPS:2: "+gpu", Was that dependency just wrong previously? A placeholder of some sort to keep the file valid? https://codereview.chromium.org/2053163002/diff/260001/components/offline_pag... File components/offline_pages/background/request_queue_store_sql.cc (right): https://codereview.chromium.org/2053163002/diff/260001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:81: // all columns present. Columnd are in order they are defined in the table. s/Columnd/Columns/, I suspect. https://codereview.chromium.org/2053163002/diff/260001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:179: " FROM " REQUEST_QUEUE_TABLE_NAME; I like this. "SELECT * " generally doesn't survive the first schema change, so being explicit from the get-go is reasonable.
Decided not to change stuff except for the comment. I think we have a plan in place for upgrade and can revisit the DB reset when we really have to. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... File components/offline_pages/background/request_queue_store_sql.cc (right): https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:83: if (!CreateTable(db, kRequestQueueTable)) On 2016/06/22 18:31:45, Scott Hess wrote: > On 2016/06/20 23:43:00, fgorski wrote: > > On 2016/06/20 19:38:33, Scott Hess wrote: > > > "CREATE TABLE IF NOT EXISTS " should work the same, here. > > > > > > Are you planning to re-use CreateTable() somewhere else? Right now you have > > > CreateSchema() which only appears to exist to wrap CreateTable() in a > > > transaction. It would seem like they could just be merged. > > > > > > [Of course, "CREATE TABLE IF NOT EXISTS " means you wouldn't need a > > transaction > > > in the first place until you add indices.] > > > > Done. I am keeping the CreateTable method separate, as we want to deal with > > upgrade/indices here. > > Since you don't have a MetaTable instance or something else tracking version > information, do you have a plan for upgrades and indices? > > I ask because my experience is that if you haven't already done a dry run for a > schema upgrade, then sometimes you will find surprising things when you need > one. Maybe not in your case, it's just that most of the nasty issues tend to be > schema changes due to the combinatorics of the problem. > > Hmm, and a comment for schema upgrades: I've found that our general system of > MetaTable-version-tracking has holes where the code to optimistically create the > current version can interact poorly with the ALTER statements. I think it's > reasonable to just have schema changes accumulate, so even new users should > start with the original schema and upgrade to current. This simplifies the set > of paths which can go wrong getting to the final schema, and also makes clear to > readers that those intermediate schema may exist in the wild. The way we think of doing schema changes is pretty simple: * Detect that a field is missing/extra * Rename the old table. (Or move data to tmp) * Create a new table with proper schema * Copy data with proper defaulting We already have one successful upgrade in the old code that you were reviewing. This works because: a) We don't expect to have a lot of data (hundreds of records top) b) We don't have relations between tables so no need to do stuff in place. c) Indexes should be simple to recreate (once we have them). Let me know if you see concerns with that. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:130: << ", SQL statement: " << statement.GetSQLStatement(); On 2016/06/22 18:31:45, Scott Hess wrote: > On 2016/06/20 23:43:00, fgorski wrote: > > On 2016/06/20 19:38:32, Scott Hess wrote: > > > Again, suggest dropping the log line. Note that if the SQL fails because of > a > > > syntax error of database corruption, there will already be logging. I > believe > > > that for COUNT(*), those are the only failures that could happen (there > should > > > _always_ be a single result, otherwise). > > > > Done. Again debugging -> I had hard time with getting the Count(*) query to > work > > and only after a while figured out that it should be using GetUniqueStatement > > with Step as opposed to Run... > > OK. I've often thought it would be nice to have a "Get a single result" helper, > but I've always cut bait because I don't want 6 of them to cover the different > types. Acknowledged. It was just not clear to me how to query for such thing. I still don't know if it would work if I used GetCachedStatement instead of Unique. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:166: : RequestQueueStore::UpdateStatus::ADDED; On 2016/06/22 18:31:45, Scott Hess wrote: > On 2016/06/20 23:43:00, fgorski wrote: > > On 2016/06/20 19:38:33, Scott Hess wrote: > > > Is tracking whether this was added or replaced really necessary? I mean > like > > > someone is going to make decisions based on it necessary? You're paying the > > > price of an explicit transaction to accomplish this, if you're doing it just > > in > > > case someone might use it someday, I'd say just skip the complication and do > > > only what you actually need now. > > > > Yeah, I don't like it, but that was requested by another developer. I can try > > persuasion... (As explained earlier, I figured out that count(*) works > > differently, so at least I got that going for me...) > > If someone is going to use it, fine. My main concern is that having the > distinction now maybe means someday someone uses it in an incidental fashion, > and later you find out that you're writing annoying workaround code to support a > use case that nobody ever really intended to support. We discussed it internally and decided against it at the store level. We'll have a higher order function that will be used to work that out before calling insertorupdate. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:282: int count = 0; On 2016/06/22 18:31:45, Scott Hess wrote: > On 2016/06/20 23:42:59, fgorski wrote: > > On 2016/06/20 19:38:33, Scott Hess wrote: > > > I usually expect countable things to be size_t. > > > > I played with the idea, however: > > a) GetLastChangeCount returns int > > b) our interface is currently set up to expect int in callback. > > > > Are you OK with this code sticking to int, at least for now? > > OK. I think the generic ruling is something like "use int or size_t". Mostly > I'm just used to STL containers using size_t, and this is iterating an STL > container. OK. Until we change the interface to be size_t I want to keep it as int. I can bring it to other developer's attention, but for now this will work for us. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:317: bool success = base::DeleteFile(db_file_path, /* recursive */ false); On 2016/06/22 18:31:44, Scott Hess wrote: > On 2016/06/20 23:43:00, fgorski wrote: > > On 2016/06/20 19:38:33, Scott Hess wrote: > > > Don't use DeleteFile(). Use sql::Connection::Delete(). > > > > > > That said, better would probably be > > > db->Raze(); // Clear all data from database. > > > db->Close(); > > > > Done. > > > > > > If you want to also delete after, that's fine :-). > > > > > > Note that there have in the past been cases where delete returning success > > > didn't actually mean delete worked. Many of those cases have been fixed, > but > > I > > > think anything which really depends on the return code from delete is > probably > > > broken or going to be broken. > > > > OK skipped deleting the file and went with Raze() > > Given the other point about this class managing a directory containing a > database, rather than a database, I guess you maybe end up with a distinction > between "Clear the data from the database" and "Delete the database". Raze() is > aiming for the first of those, if you later decide to go the delete route, or > add a delete function, IMHO delete the directory as a whole, not the files > within it. > > [That said, delete is hairy on Windows. You can have problems with virus > scanners and other malware holding handles to files and preventing the delete > from working.] The only thing I would be worried about is when the file is corrupted and I cannot open it. In that case I'd like to blow away the whole folder and start over. Otherwise (and for now) deleting the content of DB works. https://codereview.chromium.org/2053163002/diff/260001/components/offline_pag... File components/offline_pages/background/DEPS (left): https://codereview.chromium.org/2053163002/diff/260001/components/offline_pag... components/offline_pages/background/DEPS:2: "+gpu", On 2016/06/22 18:31:45, Scott Hess wrote: > Was that dependency just wrong previously? A placeholder of some sort to keep > the file valid? I think this is simply problem with diffing. I don't think we had a file here at all. https://codereview.chromium.org/2053163002/diff/260001/components/offline_pag... File components/offline_pages/background/request_queue_store_sql.cc (right): https://codereview.chromium.org/2053163002/diff/260001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:81: // all columns present. Columnd are in order they are defined in the table. On 2016/06/22 18:31:45, Scott Hess wrote: > s/Columnd/Columns/, I suspect. Done. https://codereview.chromium.org/2053163002/diff/260001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:179: " FROM " REQUEST_QUEUE_TABLE_NAME; On 2016/06/22 18:31:45, Scott Hess wrote: > I like this. "SELECT * " generally doesn't survive the first schema change, so > being explicit from the get-go is reasonable. Acknowledged. That is what I was going for. I initially started typing a comment: make sure to pay attention to the column ordering and so on... then I figured it is wrong and switched to explicit enumeration of fields in select.
LGTM still. BTW, I'm going to be out for July. I'd love to have a good backup reviewer to recommend, but nobody likes doing SQLite reviews :-(. You could try to loop in michaeln@, or he could maybe recommend someone, or just TBR me and YOLO. https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... File components/offline_pages/background/request_queue_store_sql.cc (right): https://codereview.chromium.org/2053163002/diff/200001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:130: << ", SQL statement: " << statement.GetSQLStatement(); On 2016/06/22 23:32:16, fgorski wrote: > On 2016/06/22 18:31:45, Scott Hess wrote: > > On 2016/06/20 23:43:00, fgorski wrote: > > > On 2016/06/20 19:38:32, Scott Hess wrote: > > > > Again, suggest dropping the log line. Note that if the SQL fails because > of > > a > > > > syntax error of database corruption, there will already be logging. I > > believe > > > > that for COUNT(*), those are the only failures that could happen (there > > should > > > > _always_ be a single result, otherwise). > > > > > > Done. Again debugging -> I had hard time with getting the Count(*) query to > > work > > > and only after a while figured out that it should be using > GetUniqueStatement > > > with Step as opposed to Run... > > > > OK. I've often thought it would be nice to have a "Get a single result" > helper, > > but I've always cut bait because I don't want 6 of them to cover the different > > types. > > Acknowledged. It was just not clear to me how to query for such thing. > I still don't know if it would work if I used GetCachedStatement instead of > Unique. I would err on the side of unique over cached. The concept is that you can save the overhead of re-preparing your queries. I suspect that most Chromium queries are against simple schema so the savings is not great, and seldom-enough executed that the memory-for-speed tradeoff is pretty ambiguous. https://codereview.chromium.org/2053163002/diff/260001/components/offline_pag... File components/offline_pages/background/DEPS (left): https://codereview.chromium.org/2053163002/diff/260001/components/offline_pag... components/offline_pages/background/DEPS:2: "+gpu", On 2016/06/22 23:32:16, fgorski wrote: > On 2016/06/22 18:31:45, Scott Hess wrote: > > Was that dependency just wrong previously? A placeholder of some sort to keep > > the file valid? > > I think this is simply problem with diffing. I don't think we had a file here at > all. Ha, should have checked that - it's marked A+, so it's an added file and someone is using a similarity algorithm to try to handle the moved-file case.
The CQ bit was checked by fgorski@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dougarnett@chromium.org, petewil@chromium.org Link to the patchset: https://codereview.chromium.org/2053163002/#ps280001 (title: "Fixing code comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2053163002/280001
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] Adding persistent request queue based on SQLite * This patch adds request queue SQLite base store. * Converst in memory store tests to typed tests and adapts them to SQLite. BUG=607221 ========== to ========== [Offline pages] Adding persistent request queue based on SQLite * This patch adds request queue SQLite base store. * Converst in memory store tests to typed tests and adapts them to SQLite. BUG=607221 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] Adding persistent request queue based on SQLite * This patch adds request queue SQLite base store. * Converst in memory store tests to typed tests and adapts them to SQLite. BUG=607221 ========== to ========== [Offline pages] Adding persistent request queue based on SQLite * This patch adds request queue SQLite base store. * Converst in memory store tests to typed tests and adapts them to SQLite. BUG=607221 Committed: https://crrev.com/5ff2a7b64f096594f1970eb18c61996972268835 Cr-Commit-Position: refs/heads/master@{#401622} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/5ff2a7b64f096594f1970eb18c61996972268835 Cr-Commit-Position: refs/heads/master@{#401622}
Message was sent while issue was closed.
pasko@chromium.org changed reviewers: + pasko@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2053163002/diff/280001/components/offline_pag... File components/offline_pages/background/request_queue_store_sql.cc (right): https://codereview.chromium.org/2053163002/diff/280001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:132: db->set_cache_size(500); curious: did the default values not work here for some particular reason or it is just a copy-paste from offline_page_metadata_store_sql.cc?
Message was sent while issue was closed.
Hi, Egor, I was reusing a lot of offline_page_metadata_store_sql, to keep the stores as similar as possible. I actually read through extensive comments that Scott left when bburns@ was developing the first store. Please see the pasted chunk that is relevant to your question, or follow the link to get more information. Are you working on a store of your own right now? Is there any way I could help you? Thanks. Filip https://codereview.chromium.org/2053163002/diff/280001/components/offline_pag... File components/offline_pages/background/request_queue_store_sql.cc (right): https://codereview.chromium.org/2053163002/diff/280001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:132: db->set_cache_size(500); On 2017/03/31 12:29:16, pasko wrote: > curious: did the default values not work here for some particular reason or it > is just a copy-paste from offline_page_metadata_store_sql.cc? This is following recommendation from shess@ made in https://codereview.chromium.org/1834563002/diff/490001/components/offline_pag... ''' I just realized that I was being lax about other connection setup! Somewhere around here you should have something like: db_->set_page_size(4096); db_->set_cache_size(500); // Or -2000 db_->set_histogram_tag("OfflinePageMetadata"); db_->set_exclusive_locking(); The default page size is 1024, but SQLite is changing to 4096. The vast majority of our systems should use 4096 going forward. 4096 is somewhat more efficient with read-ahead and page packing, 1024 is probably better for very constrained systems, but the primary reason for the change is that 1024 on many systems to day involves a read-modify-write cycle. For your case, I think your rows are likely to be large enough to justify this, because it should reduce the incidence of overflow pages quite a bit. The default cache size is 2000 pages, so ~2MB for 1024-byte pages. So page_size*4 implies cache_size/4. Negative values tell SQLite to calculate in terms of KB rather than page units. [Maybe I should add a set_cache_size_kb() or something and deprecate the prior usage.] [[These days by default the file is mmap'ed which mostly removes the need to worry too much about the cache size.]] ... [More stuff here] '''
Message was sent while issue was closed.
Filip, thank you for detailed response!! On 2017/03/31 16:15:22, fgorski wrote: > Hi, Egor, > > I was reusing a lot of offline_page_metadata_store_sql, to keep the stores as > similar as possible. I actually read through extensive comments that Scott left > when bburns@ was developing the first store. Please see the pasted chunk that is > relevant to your question, or follow the link to get more information. yeah, makes sense > Are you working on a store of your own right now? Is there any way I could help > you? I am investigating how our various databases behave on Android to potentially reduce the amount of stuff we write (http://crbug.com/697122). This DB is by far not near the biggest offenders :) This question is only related because as part of the investigation I occasionally glanced here and wanted to self-educate. > https://codereview.chromium.org/2053163002/diff/280001/components/offline_pag... > File components/offline_pages/background/request_queue_store_sql.cc (right): > > https://codereview.chromium.org/2053163002/diff/280001/components/offline_pag... > components/offline_pages/background/request_queue_store_sql.cc:132: > db->set_cache_size(500); > On 2017/03/31 12:29:16, pasko wrote: > > curious: did the default values not work here for some particular reason or it > > is just a copy-paste from offline_page_metadata_store_sql.cc? > > This is following recommendation from shess@ made in > https://codereview.chromium.org/1834563002/diff/490001/components/offline_pag... > > ''' > I just realized that I was being lax about other connection setup! Somewhere > around here you should have something like: > db_->set_page_size(4096); > db_->set_cache_size(500); // Or -2000 > db_->set_histogram_tag("OfflinePageMetadata"); > db_->set_exclusive_locking(); > > The default page size is 1024, but SQLite is changing to 4096. The vast > majority of our systems should use 4096 going forward. 4096 is somewhat more > efficient with read-ahead and page packing, 1024 is probably better for very > constrained systems, but the primary reason for the change is that 1024 on many > systems to day involves a read-modify-write cycle. For your case, I think your > rows are likely to be large enough to justify this, because it should reduce the > incidence of overflow pages quite a bit. hm, is this assumption the same for both databases? seems like request_queue may operate on smaller chunks? Anyhow, changing it would be premature at this point I think. |
