Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10989063:
Changed DB to store node positions as Ordinals. (Closed)
|
Created:
8 years, 2 months ago by vishwath Modified:
8 years, 2 months ago CC:
chromium-reviews, Raghu Simha, haitaol1, tim (not reviewing) Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionChanged DB to store node positions as Ordinals.
As part of the effort to move away from int64 based node positions,
changed the DB to store server_position_in_node as an ordinal
(represented by a varchar) instead. Also updated unittests and the
latest db version number (now at 81).
BUG=145412
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=160774
Patch Set 1 #
Total comments: 2
Patch Set 2 : Changed field from varchar to blob, improved tests #
Total comments: 5
Patch Set 3 : Fixed errors and corrected migration logic. #
Total comments: 23
Patch Set 4 : Implemented feedback changes. #
Total comments: 41
Patch Set 5 : Minor changes and corrections #
Total comments: 7
Patch Set 6 : Undid sql/ changes and changed column name #Patch Set 7 : Minor fixes #
Total comments: 8
Patch Set 8 : Changed server_position to server_ordinal in DB #
Total comments: 43
Patch Set 9 : Invalid ordinals handling and minor fixes #
Total comments: 1
Patch Set 10 : More fixes #Patch Set 11 : Ordinal default ctor now creates invalid Ordinals #
Total comments: 45
Patch Set 12 : Added SetUpVersion81Database() fn. #
Total comments: 15
Patch Set 13 : Minor fixes #
Total comments: 4
Patch Set 14 : Moar minor changes #Patch Set 15 : Fixed memory leak error in unittest #
Messages
Total messages: 50 (0 generated)
I'm not entirely sure that I've covered the intended scope of #2, so feel free to hurl advice/abuse/praise as appropriate.
Some initial comments. https://codereview.chromium.org/10989063/diff/1/sync/syncable/directory_backi... File sync/syncable/directory_backing_store_unittest.cc (right): https://codereview.chromium.org/10989063/diff/1/sync/syncable/directory_backi... sync/syncable/directory_backing_store_unittest.cc:2086: // Unfortunately there isn't a SetUpVersion80Database to use You should fix this. I believe it was Nicolas who added the bag of chips. You should ask him about its effects on the database format. https://codereview.chromium.org/10989063/diff/1/sync/syncable/directory_backi... sync/syncable/directory_backing_store_unittest.cc:2105: ASSERT_EQ(s.ColumnType(3), COLUMN_TYPE_TEXT); Shouldn't you check the values stored in the column aren't corrupted by the migration, or is that handled elsewhere?
Sure, I'll find out from Nicholas and get have that fixed. Would that be considered a part of the same issue or should I fork a new branch for it? Also, I haven't handled checking for corruption because my understanding was that the bigint -> varchar conversion was essentially lossless. Still it seems prudent to check anyway so maybe I should add that in there. Let me do that right away. On Thu, Sep 27, 2012 at 4:10 PM, <rlarocque@chromium.org> wrote: > Some initial comments. > > > https://codereview.chromium.**org/10989063/diff/1/sync/** > syncable/directory_backing_**store_unittest.cc<https://codereview.chromium.org/10989063/diff/1/sync/syncable/directory_backing_store_unittest.cc> > File sync/syncable/directory_**backing_store_unittest.cc (right): > > https://codereview.chromium.**org/10989063/diff/1/sync/** > syncable/directory_backing_**store_unittest.cc#newcode2086<https://codereview.chromium.org/10989063/diff/1/sync/syncable/directory_backing_store_unittest.cc#newcode2086> > sync/syncable/directory_**backing_store_unittest.cc:**2086: // > Unfortunately > > there isn't a SetUpVersion80Database to use > You should fix this. I believe it was Nicolas who added the bag of > chips. You should ask him about its effects on the database format. > > https://codereview.chromium.**org/10989063/diff/1/sync/** > syncable/directory_backing_**store_unittest.cc#newcode2105<https://codereview.chromium.org/10989063/diff/1/sync/syncable/directory_backing_store_unittest.cc#newcode2105> > sync/syncable/directory_**backing_store_unittest.cc:**2105: > ASSERT_EQ(s.ColumnType(3), COLUMN_TYPE_TEXT); > Shouldn't you check the values stored in the column aren't corrupted by > the migration, or is that handled elsewhere? > > https://codereview.chromium.**org/10989063/<https://codereview.chromium.org/1... >
I've changed the field from a varchar to a blob. Also added the SetupVersion80Database() function needed by the unittests. The unit test for 81 now checks for corruption as well.
https://codereview.chromium.org/10989063/diff/5001/sync/syncable/directory_ba... File sync/syncable/directory_backing_store.cc (right): https://codereview.chromium.org/10989063/diff/5001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:1018: "ALTER COLUMN server_position_in_parent VARCHAR(255)")) { This is should be a BLOB, right? https://codereview.chromium.org/10989063/diff/5001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:1019: return false; I think you need to convert the values, too. https://codereview.chromium.org/10989063/diff/5001/sync/syncable/directory_ba... File sync/syncable/directory_backing_store_unittest.cc (right): https://codereview.chromium.org/10989063/diff/5001/sync/syncable/directory_ba... sync/syncable/directory_backing_store_unittest.cc:1635: // Is that intentional or should it be fixed? I'd fix it. https://codereview.chromium.org/10989063/diff/5001/sync/syncable/directory_ba... sync/syncable/directory_backing_store_unittest.cc:2201: ASSERT_EQ("1048576", s.ColumnString); I don't think this is right. Wouldn't the int64 value 1048756 translate into a bunch of binary data, rather than the string "1048756"? https://codereview.chromium.org/10989063/diff/5001/sync/syncable/directory_ba... sync/syncable/directory_backing_store_unittest.cc:2203: Given the likelihood of a regression, I think you should create a new test that saves an ordinal whose internal value contains a NULL character into the database then reads it back again.
I think it should be fine now.
Some comments. http://codereview.chromium.org/10989063/diff/5002/sync/syncable/directory_bac... File sync/syncable/directory_backing_store.cc (right): http://codereview.chromium.org/10989063/diff/5002/sync/syncable/directory_bac... sync/syncable/directory_backing_store.cc:54: // to using ordinals instead of int64's nit: put period after comment. Also, you might want to reference the bug number here. http://codereview.chromium.org/10989063/diff/5002/sync/syncable/directory_bac... sync/syncable/directory_backing_store.cc:55: if(i == SERVER_POSITION_IN_PARENT) { nit: space between if and parenthesis. http://codereview.chromium.org/10989063/diff/5002/sync/syncable/directory_bac... sync/syncable/directory_backing_store.cc:59: continue; I'd prefer an 'else' branch to 'continue'. http://codereview.chromium.org/10989063/diff/5002/sync/syncable/directory_bac... sync/syncable/directory_backing_store.cc:94: statement->ColumnType(i) == sql::COLUMN_TYPE_BLOB) { We shouldn't get here unless the database is migrated, right? Maybe the check for ColumnType(i) should be a DCHECK_EQ()? Would the sql functions DCHECK on you if it wasn't a blob? Maybe you should let them catch the error failure. http://codereview.chromium.org/10989063/diff/5002/sync/syncable/directory_bac... sync/syncable/directory_backing_store.cc:99: kernel->put(static_cast<Int64Field>(i), server_position); nit: I'd prefer that the first argument to put be SERVER_POSITION_IN_PARENT because I think that's more readable, though I admit it doesn't make much difference either way. http://codereview.chromium.org/10989063/diff/5002/sync/syncable/directory_bac... sync/syncable/directory_backing_store.cc:100: continue; I'd prefer an else branch here. http://codereview.chromium.org/10989063/diff/5002/sync/syncable/directory_bac... sync/syncable/directory_backing_store.cc:1016: // bigint to an unbounded fidelity ordinal (stored as a blob) I wouldn't call it a bigint, that term could be overloaded. Let's just say it's an int64. http://codereview.chromium.org/10989063/diff/5002/sync/syncable/directory_bac... sync/syncable/directory_backing_store.cc:1020: SafeDropTable("temp_metas"); Is there a transaction open at this point? If not, we should open one. This could do a lot of damage if we crash when migration is half done. http://codereview.chromium.org/10989063/diff/5002/sync/syncable/directory_bac... sync/syncable/directory_backing_store.cc:1028: std::string old_info_string = "SELECT "; I was hoping we could perform the migration without forcing every value in the database to round-trip through C++. This might be naive, but here's what I had in mind: 1. Rename SERVER_POSITION_IN_PARENT to OLD_SERVER_POSITION_IN_PARENT. 2. Add a new SERVER_POSITION_IN_PARENT column of type BLOB 3. Extract pairs of (metahandle (ie. primary key), OLD_SERVER_POSITION_IN_PARENT) 4. Perform the conversion 5. Insert the data into SERVER_POSITION_IN_PARENT 6. Have RefreshColumns() clean up the old, temporary column. http://codereview.chromium.org/10989063/diff/5002/sync/syncable/directory_bac... sync/syncable/directory_backing_store.cc:1035: for(int i = BEGIN_FIELDS; i < BEGIN_FIELDS + FIELD_COUNT - 1; ++i) { I'd really like to avoid this sort of thing, though I have an unusually strong dislike of code that pieces together SQL queries through string manipulation. Others may not share my disapproval. http://codereview.chromium.org/10989063/diff/5002/sync/syncable/directory_bac... File sync/syncable/directory_backing_store_unittest.cc (right): http://codereview.chromium.org/10989063/diff/5002/sync/syncable/directory_bac... sync/syncable/directory_backing_store_unittest.cc:2186: sql::Statement s(connection.GetUniqueStatement( You might be able to implement this with PRAGMA TABLE_INFO. I'm unsure whether or not that's cleaner than this, though it does have the advantage that it would work even on an empty table. Maybe we should extend the connection.h API to have a function for this, similar to DoesColumnExist? http://codereview.chromium.org/10989063/diff/5002/sync/syncable/directory_bac... sync/syncable/directory_backing_store_unittest.cc:2208: // Next, test that null characters in the I think you should split this out into a separate test, if possible. In fact, I think it would work better if it were to write it at a higher level. syncable_unittest.cc has a helper function called SimulateSaveAndReloadDir(). If we could contrive an int64 value known to produce NULL values in its ordinal representation, and to reload differently if they are misinterpreted, then we could test that save and restore works using the actual SQL statements used by the DirectoryBackingStore in practice.
Implemented most of the feedback.
Also, some of the changes affect sql/connection{h,cc,unittest} and
sql/statement.h. Who would be the best person to LGTM those changes?
http://codereview.chromium.org/10989063/diff/5002/sync/syncable/directory_bac...
File sync/syncable/directory_backing_store.cc (right):
http://codereview.chromium.org/10989063/diff/5002/sync/syncable/directory_bac...
sync/syncable/directory_backing_store.cc:54: // to using ordinals instead of
int64's
On 2012/09/29 01:13:42, rlarocque wrote:
> nit: put period after comment.
>
> Also, you might want to reference the bug number here.
Done.
http://codereview.chromium.org/10989063/diff/5002/sync/syncable/directory_bac...
sync/syncable/directory_backing_store.cc:55: if(i == SERVER_POSITION_IN_PARENT)
{
On 2012/09/29 01:13:42, rlarocque wrote:
> nit: space between if and parenthesis.
Done.
http://codereview.chromium.org/10989063/diff/5002/sync/syncable/directory_bac...
sync/syncable/directory_backing_store.cc:59: continue;
On 2012/09/29 01:13:42, rlarocque wrote:
> I'd prefer an 'else' branch to 'continue'.
Done.
http://codereview.chromium.org/10989063/diff/5002/sync/syncable/directory_bac...
sync/syncable/directory_backing_store.cc:99:
kernel->put(static_cast<Int64Field>(i), server_position);
On 2012/09/29 01:13:42, rlarocque wrote:
> nit: I'd prefer that the first argument to put be SERVER_POSITION_IN_PARENT
> because I think that's more readable, though I admit it doesn't make much
> difference either way.
Done.
http://codereview.chromium.org/10989063/diff/5002/sync/syncable/directory_bac...
sync/syncable/directory_backing_store.cc:100: continue;
On 2012/09/29 01:13:42, rlarocque wrote:
> I'd prefer an else branch here.
Done.
http://codereview.chromium.org/10989063/diff/5002/sync/syncable/directory_bac...
sync/syncable/directory_backing_store.cc:1016: // bigint to an unbounded
fidelity ordinal (stored as a blob)
On 2012/09/29 01:13:42, rlarocque wrote:
> I wouldn't call it a bigint, that term could be overloaded. Let's just say
it's
> an int64.
Done.
http://codereview.chromium.org/10989063/diff/5002/sync/syncable/directory_bac...
sync/syncable/directory_backing_store.cc:1020: SafeDropTable("temp_metas");
I think all the Migrate*() functions rely on the calling functions to set the
transaction up (either InitializeTables() or the appropriate unittest).
On 2012/09/29 01:13:42, rlarocque wrote:
> Is there a transaction open at this point? If not, we should open one. This
> could do a lot of damage if we crash when migration is half done.
http://codereview.chromium.org/10989063/diff/5002/sync/syncable/directory_bac...
sync/syncable/directory_backing_store.cc:1028: std::string old_info_string =
"SELECT ";
That'a actually the way I'd prefer to do it as well. Unfortunately, sqlite does
not have a column renaming command (alter table is far more restrictive) so it
seems we're forced to do this round about approach.
On 2012/09/29 01:13:42, rlarocque wrote:
> I was hoping we could perform the migration without forcing every value in the
> database to round-trip through C++.
>
> This might be naive, but here's what I had in mind:
> 1. Rename SERVER_POSITION_IN_PARENT to OLD_SERVER_POSITION_IN_PARENT.
> 2. Add a new SERVER_POSITION_IN_PARENT column of type BLOB
> 3. Extract pairs of (metahandle (ie. primary key),
> OLD_SERVER_POSITION_IN_PARENT)
> 4. Perform the conversion
> 5. Insert the data into SERVER_POSITION_IN_PARENT
> 6. Have RefreshColumns() clean up the old, temporary column.
http://codereview.chromium.org/10989063/diff/5002/sync/syncable/directory_bac...
sync/syncable/directory_backing_store.cc:1035: for(int i = BEGIN_FIELDS; i <
BEGIN_FIELDS + FIELD_COUNT - 1; ++i) {
I see the inherent vulnerabilities in misusing this approach, so if you feel
strongly enough about it I can do this the hard way.
On 2012/09/29 01:13:42, rlarocque wrote:
> I'd really like to avoid this sort of thing, though I have an unusually strong
> dislike of code that pieces together SQL queries through string manipulation.
> Others may not share my disapproval.
http://codereview.chromium.org/10989063/diff/5002/sync/syncable/directory_bac...
File sync/syncable/directory_backing_store_unittest.cc (right):
http://codereview.chromium.org/10989063/diff/5002/sync/syncable/directory_bac...
sync/syncable/directory_backing_store_unittest.cc:2186: sql::Statement
s(connection.GetUniqueStatement(
On 2012/09/29 01:13:42, rlarocque wrote:
> You might be able to implement this with PRAGMA TABLE_INFO. I'm unsure
whether
> or not that's cleaner than this, though it does have the advantage that it
would
> work even on an empty table.
>
> Maybe we should extend the connection.h API to have a function for this,
similar
> to DoesColumnExist?
Done.
http://codereview.chromium.org/10989063/diff/5002/sync/syncable/directory_bac...
sync/syncable/directory_backing_store_unittest.cc:2208: // Next, test that null
characters in the
On 2012/09/29 01:13:42, rlarocque wrote:
> I think you should split this out into a separate test, if possible.
>
> In fact, I think it would work better if it were to write it at a higher
level.
> syncable_unittest.cc has a helper function called SimulateSaveAndReloadDir().
> If we could contrive an int64 value known to produce NULL values in its
ordinal
> representation, and to reload differently if they are misinterpreted, then we
> could test that save and restore works using the actual SQL statements used by
> the DirectoryBackingStore in practice.
Done.
The owners of the SQL code are listed in sql/OWNERS file. I did a random sort and erikwright came out first. +erikwright: Please help review the sql changes. We can split them off into a separate patch if desired, though I thought you might find it easier to review with some context. ---- As an overall comment, I'm still not comfortable with the way migration is being handled. We might be able to fix it by manually specifying all columns in v80 and v81 databases, but that's not a great solution, either. I haven't fully thought this through, but I wonder if this might be easier if we allowed the column name to change. Would that make it possible to write simpler migration code? http://codereview.chromium.org/10989063/diff/15001/sql/connection.cc File sql/connection.cc (right): http://codereview.chromium.org/10989063/diff/15001/sql/connection.cc#newcode15 sql/connection.cc:15: #include "sql/statement.h" This should be in alphabetical order. http://codereview.chromium.org/10989063/diff/15001/sql/connection.cc#newcode468 sql/connection.cc:468: std::string column_type = statement.ColumnString(2); When I suggested the PRAGMA TABLE INFO, it didn't cross my mind that this would result in string compares here. sqlite3_table_column_metadata() might provide an easier to use alternative. http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... File sync/syncable/directory_backing_store.cc (right): http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:30: #include "sync/internal_api/public/base/ordinal.h" These should be in alphabetical order. http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:54: // to using ordinals instead of int64's Comments should end with a period. http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:61: else That's unusual formatting. You should put the else on the same line as the closing brace, and put the else block in braces. http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:93: // to using ordinals instead of int64's //VISH_TODO: CONTINUE HERE Did you intend to leave this comment here? http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:95: statement->ColumnType(i) == sql::COLUMN_TYPE_BLOB) { Make this second expression a DCHECK? We shouldn't get here unless the database is at the proper version, and the column type will be blob if the database was correctly migrated. http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:101: static_cast<Int64Field>(SERVER_POSITION_IN_PARENT), The parameters to put() should be aligned. http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:104: else Change this formatting, too. http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:1019: // int64 to an unbounded fidelity ordinal (stored as a blob) Append period here. http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:1024: if(!db_->Execute( Does this not fit on one line? http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:1030: Remove extra whitespace. http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:1033: AppendColumnList(&old_info_string); This is dangerous. Someone might update g_metas_columns without realizing that it's called in this context with the implicit assumption that it represents the columns of a v80 database. http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:1037: AppendColumnList(&new_info_string); Same warning here, except that this call expects the columns to match a v81 database. http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:1039: for(int i = BEGIN_FIELDS; i < BEGIN_FIELDS + FIELD_COUNT - 1; ++i) { Similar warnings apply to this loop, too. And other places in the rest of this function... http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... File sync/syncable/directory_backing_store_unittest.cc (right): http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... sync/syncable/directory_backing_store_unittest.cc:25: #include "sync/internal_api/public/base/ordinal.h" These should be in alphabetical order. http://codereview.chromium.org/10989063/diff/15001/sync/syncable/syncable_uni... File sync/syncable/syncable_unittest.cc (right): http://codereview.chromium.org/10989063/diff/15001/sync/syncable/syncable_uni... sync/syncable/syncable_unittest.cc:38: #include "sync/internal_api/public/base/ordinal.h" Fix include ordering. http://codereview.chromium.org/10989063/diff/15001/sync/syncable/syncable_uni... sync/syncable/syncable_unittest.cc:1375: NodeOrdinal("Thisshould\0workfine")); IIRC, only the first 8 bytes will be maintained after the NodeOrdinalToInt64() conversion. So, after the conversion, "Thisshould\0workfine" == "Thisshou". That means the '\0' handling might not be well tested. Other than this value, though, the test looks great.
+shess, the preferred reviewer for sql/ changes unless he's absent. -me
http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... File sync/syncable/directory_backing_store.cc (right): http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:54: // to using ordinals instead of int64's int64's int64s http://codereview.chromium.org/10989063/diff/15001/sync/syncable/syncable_col... File sync/syncable/syncable_columns.h (right): http://codereview.chromium.org/10989063/diff/15001/sync/syncable/syncable_col... sync/syncable/syncable_columns.h:25: {"server_version", "blob"}, why is server_version a blob also?
I think I've fixed everything (famous last words I presume) http://codereview.chromium.org/10989063/diff/15001/sql/connection.cc File sql/connection.cc (right): http://codereview.chromium.org/10989063/diff/15001/sql/connection.cc#newcode15 sql/connection.cc:15: #include "sql/statement.h" On 2012/10/01 21:45:55, rlarocque wrote: > This should be in alphabetical order. Done. http://codereview.chromium.org/10989063/diff/15001/sql/connection.cc#newcode468 sql/connection.cc:468: std::string column_type = statement.ColumnString(2); It turns out that the returned information is of a higher granularity than the 'fundamental' types (integer, text, blob, null). I've changed this to just return the exact data type (bigint, varchar, etc), so that its now the callers responsibility to make semantic sense of that return value. I see this as an advantage, since the callers of this function can use that extra information if necessary. On 2012/10/01 21:45:55, rlarocque wrote: > When I suggested the PRAGMA TABLE INFO, it didn't cross my mind that this would > result in string compares here. > > sqlite3_table_column_metadata() might provide an easier to use alternative. http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... File sync/syncable/directory_backing_store.cc (right): http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:30: #include "sync/internal_api/public/base/ordinal.h" On 2012/10/01 21:45:55, rlarocque wrote: > These should be in alphabetical order. Done. http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:54: // to using ordinals instead of int64's On 2012/10/01 21:45:55, rlarocque wrote: > Comments should end with a period. Done. http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:54: // to using ordinals instead of int64's On 2012/10/01 23:13:35, akalin wrote: > int64's int64s Done. http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:61: else An unfortunate take-away from too much python programming. Done and remembered. On 2012/10/01 21:45:55, rlarocque wrote: > That's unusual formatting. You should put the else on the same line as the > closing brace, and put the else block in braces. http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:93: // to using ordinals instead of int64's //VISH_TODO: CONTINUE HERE That would be an embarrassed no. On 2012/10/01 21:45:55, rlarocque wrote: > Did you intend to leave this comment here? http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:95: statement->ColumnType(i) == sql::COLUMN_TYPE_BLOB) { I changed the check to a test for a non-null column. I figured that the easiest way to handle null values was to leave them as is. On 2012/10/01 21:45:55, rlarocque wrote: > Make this second expression a DCHECK? We shouldn't get here unless the database > is at the proper version, and the column type will be blob if the database was > correctly migrated. http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:101: static_cast<Int64Field>(SERVER_POSITION_IN_PARENT), On 2012/10/01 21:45:55, rlarocque wrote: > The parameters to put() should be aligned. Done. http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:104: else On 2012/10/01 21:45:55, rlarocque wrote: > Change this formatting, too. Done. http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:1019: // int64 to an unbounded fidelity ordinal (stored as a blob) On 2012/10/01 21:45:55, rlarocque wrote: > Append period here. Done. http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:1024: if(!db_->Execute( On 2012/10/01 21:45:55, rlarocque wrote: > Does this not fit on one line? Done. http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:1030: On 2012/10/01 21:45:55, rlarocque wrote: > Remove extra whitespace. Done. http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:1033: AppendColumnList(&old_info_string); On 2012/10/01 21:45:55, rlarocque wrote: > This is dangerous. Someone might update g_metas_columns without realizing that > it's called in this context with the implicit assumption that it represents the > columns of a v80 database. Done. http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:1037: AppendColumnList(&new_info_string); On 2012/10/01 21:45:55, rlarocque wrote: > Same warning here, except that this call expects the columns to match a v81 > database. Done. http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:1039: for(int i = BEGIN_FIELDS; i < BEGIN_FIELDS + FIELD_COUNT - 1; ++i) { On 2012/10/01 21:45:55, rlarocque wrote: > Similar warnings apply to this loop, too. And other places in the rest of this > function... Done. http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... File sync/syncable/directory_backing_store_unittest.cc (right): http://codereview.chromium.org/10989063/diff/15001/sync/syncable/directory_ba... sync/syncable/directory_backing_store_unittest.cc:25: #include "sync/internal_api/public/base/ordinal.h" On 2012/10/01 21:45:55, rlarocque wrote: > These should be in alphabetical order. Done. http://codereview.chromium.org/10989063/diff/15001/sync/syncable/syncable_col... File sync/syncable/syncable_columns.h (right): http://codereview.chromium.org/10989063/diff/15001/sync/syncable/syncable_col... sync/syncable/syncable_columns.h:25: {"server_version", "blob"}, Not sure how that crept in, fixed. On 2012/10/01 23:13:35, akalin wrote: > why is server_version a blob also? http://codereview.chromium.org/10989063/diff/15001/sync/syncable/syncable_uni... File sync/syncable/syncable_unittest.cc (right): http://codereview.chromium.org/10989063/diff/15001/sync/syncable/syncable_uni... sync/syncable/syncable_unittest.cc:38: #include "sync/internal_api/public/base/ordinal.h" On 2012/10/01 21:45:55, rlarocque wrote: > Fix include ordering. Done. http://codereview.chromium.org/10989063/diff/15001/sync/syncable/syncable_uni... sync/syncable/syncable_unittest.cc:1375: NodeOrdinal("Thisshould\0workfine")); On 2012/10/01 21:45:55, rlarocque wrote: > IIRC, only the first 8 bytes will be maintained after the NodeOrdinalToInt64() > conversion. > > So, after the conversion, "Thisshould\0workfine" == "Thisshou". That means the > '\0' handling might not be well tested. > > Other than this value, though, the test looks great. Done.
A few more comments. I only skimmed the SQL section, since we're waiting for shess's comments there. I've re-added him to the reviewers list, btw. I'm not sure how Erik's changes came undone. I only skimmed the migration parts as well, since that part is likely to change as we discussed earlier. http://codereview.chromium.org/10989063/diff/15001/sql/connection.cc File sql/connection.cc (right): http://codereview.chromium.org/10989063/diff/15001/sql/connection.cc#newcode468 sql/connection.cc:468: std::string column_type = statement.ColumnString(2); On 2012/10/02 01:06:33, vishwath wrote: > It turns out that the returned information is of a higher granularity than the > 'fundamental' types (integer, text, blob, null). I've changed this to just > return the exact data type (bigint, varchar, etc), so that its now the callers > responsibility to make semantic sense of that return value. I see this as an > advantage, since the callers of this function can use that extra information if > necessary. > Not quite what I had in mind, but that looks OK. I'll leave it to the SQL expert to decide if this makes sense. http://codereview.chromium.org/10989063/diff/20001/sync/syncable/directory_ba... File sync/syncable/directory_backing_store.cc (right): http://codereview.chromium.org/10989063/diff/20001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:60: } else Opening brace should be on the same line. http://codereview.chromium.org/10989063/diff/20001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:95: // Columns with a null value for the ordinal (like the root node) If you're going to have to special case it here, then I think I would prefer if the root node's SERVER_POSITION_IN_PARENT was simply not NULL to begin with. Is it correct to allow the root node's SERVER_POSITION_IN_PARENT value to be set as an int64 in the else branch? What will happen to it when we eventually switch to using ordinals in-memory, too? http://codereview.chromium.org/10989063/diff/20001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:106: } else Opening brace should be on the same line. http://codereview.chromium.org/10989063/diff/20001/sync/syncable/syncable_uni... File sync/syncable/syncable_unittest.cc (right): http://codereview.chromium.org/10989063/diff/20001/sync/syncable/syncable_uni... sync/syncable/syncable_unittest.cc:1375: std::string null_str(null_cstr, null_cstr + 9); That second parameter is likely to be bigger than you expected. I think, but I'm not entirely sure, that sizeof(null_cstr) will get you the value you want. Maybe subtract 1 from it if you want to be really tricky and pass in a string that is not NULL-terminated.
I'm not sure I understand why you need to change sql/ at all. AFAICT, you're moving stuff around that doesn't need to move around, and then providing a thin wrapper around pragma table_info which doesn't actually enforce anything. We have lots of people who write stuff like MEDIUMCHAR and BIGINT. These constructs have no real meaning in SQLite, and since we will never use a different backend, we should not encourage users of the API to depart from the core SQLite types. Doing so is misleading at best, error-prone at worst. If your code insists on making up shades of types which don't actually make sense to SQLite, I have no problem with having your code make direct PRAGMA calls. I just prefer not to pull them into sql/ unless they're tightly constrained. Well, and also I'm generally against dynamically-constructed statements. They work just fine 9 times out of 10, and the 10th time generates a bug which wastes far more developer time than simply coding it out longhand in the first place. http://codereview.chromium.org/10989063/diff/20001/sql/connection.cc File sql/connection.cc (right): http://codereview.chromium.org/10989063/diff/20001/sql/connection.cc#newcode16 sql/connection.cc:16: Egregious whitespace change is egregious. http://codereview.chromium.org/10989063/diff/20001/sql/connection.h File sql/connection.h (right): http://codereview.chromium.org/10989063/diff/20001/sql/connection.h#newcode35 sql/connection.h:35: }; AFAICT, this is not used in this file? Either that or GetColumnType is returning the wrong value. http://codereview.chromium.org/10989063/diff/20001/sql/connection_unittest.cc File sql/connection_unittest.cc (right): http://codereview.chromium.org/10989063/diff/20001/sql/connection_unittest.cc... sql/connection_unittest.cc:118: EXPECT_EQ(db().GetColumnType(table_name, 1), "varchar"); "bigint" and "varchar" are meaningless to SQLite, these are INTEGER and TEXT. The fact that you need to depend on these means that you've been doing something wrong. I don't think it is appropriate to encourage people to expect GetColumnType() to return anything other than the five native types.
Changes to sql/ have been removed. (I've removed shess from the reviewer list but will mail him separately to let him know) I've changed column names, but haven't been able to use RefreshColumns() to simplify the transfer process. That's because there seems to be an issue with RefreshColumns() and InitializeTables() (when its called by dbs->Load() in a test) - tables aren't being dropped. You can reproduce this by setting needs_column_refresh = true in any migration after 79 (the 79->79 migration unit test is the first occurrence of the dbs->Load() call). I thought the error might be better fixed as a separate issue?
I think you should look into why RefreshColumns() isn't working. Which columns does it fail to drop, and why? http://codereview.chromium.org/10989063/diff/30001/sync/syncable/directory_ba... File sync/syncable/directory_backing_store.cc (right): http://codereview.chromium.org/10989063/diff/30001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:55: // See crbug.com/145412 I think there should be a period here, too. http://codereview.chromium.org/10989063/diff/30001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:56: if (i == SERVER_POSITION_IN_PARENT) { What will we do when SERVER_POSITION_IN_PARENT is no longer an int64? I don't think we want this column to live among the ints forever. Let's create a new SERVER_POSITION_ORDINAL enum (or whatever it is we decide to call it) of "type" ORDINAL_FIELD and put the new ordinal column there. We'll still need the special-case to populate SERVER_POSITION_IN_PARENT with an int64 value for now, but we'll be able to remove it eventually.
http://codereview.chromium.org/10989063/diff/30001/sync/syncable/directory_ba... File sync/syncable/directory_backing_store.cc (right): http://codereview.chromium.org/10989063/diff/30001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:25: #include "sync/internal_api/public/base/ordinal.h" i think you can include just node_ordinal.h, and not ordinal.h http://codereview.chromium.org/10989063/diff/30001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:55: // See crbug.com/145412 On 2012/10/02 21:48:21, rlarocque wrote: > I think there should be a period here, too. preferably with a space before (so the link doesn't get messed up). http://codereview.chromium.org/10989063/diff/30001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:56: if (i == SERVER_POSITION_IN_PARENT) { On 2012/10/02 21:48:21, rlarocque wrote: > What will we do when SERVER_POSITION_IN_PARENT is no longer an int64? I don't > think we want this column to live among the ints forever. > > Let's create a new SERVER_POSITION_ORDINAL enum (or whatever it is we decide to > call it) of "type" ORDINAL_FIELD and put the new ordinal column there. We'll > still need the special-case to populate SERVER_POSITION_IN_PARENT with an int64 > value for now, but we'll be able to remove it eventually. I think we should get rid of SERVER_POSITION_IN_PARENT, actually, and do the int64<->NodeOrdinal conversion whenever we use SERVER_POSITION_IN_PARENT now. What do you think? Since the column list in syncable_columns.h must match the field order, I think we either need to keep both columns or get rid of SERVER_POSITION_IN_PARENT. http://codereview.chromium.org/10989063/diff/30001/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:1090: bool DirectoryBackingStore::MigrateVersion80To81() { whoa, this is scary. might be a stupid question, but why can't we do something like: ALTER TABLE metas ADD COLUMN server_ordinal_in_parent; // Loop through table, build ordinals from server_position_in_parent ALTER TABLE metas DROP COLUMN server_position_in_parent; ? (I guess this would depend on my other comments.) http://codereview.chromium.org/10989063/diff/30001/sync/syncable/syncable_col... File sync/syncable/syncable_columns.h (right): http://codereview.chromium.org/10989063/diff/30001/sync/syncable/syncable_col... sync/syncable/syncable_columns.h:19: // Must be in exact same order as fields in syncable. since you're here, change 'syncable' to 'entry_kernel.h'. http://codereview.chromium.org/10989063/diff/30001/sync/syncable/syncable_col... sync/syncable/syncable_columns.h:26: {"server_ordinal_in_parent", "blob"}, i think this should be moved down to the blobs section below. In fact, to differentiate them from the other fields (which are serialized protos), maybe something like: // Blobs (serialized protos). {"specifics"...} ... // Blobs (ordinals). {"server_ordinal"...}, (Might be easiest to put the column last).
Having the freedom to use a different column has made the migration logic for
80->81 vastly simpler. I've had to disable parts of two other unittests though,
due to a table locking issue that results from the
TestDirectoryBackingStore::Load() --> RefreshColumn() --> SafeDropTable("metas")
call chain. The table ends up not being dropped and a subsequent attempt to
rename temp_metas to metas crashes. I've left some todos in there to resolve the
locking issue, but thought it would make more sense to work on that after
publishing this for review.
moar comments http://codereview.chromium.org/10989063/diff/22004/sync/engine/build_commit_c... File sync/engine/build_commit_command.cc (right): http://codereview.chromium.org/10989063/diff/22004/sync/engine/build_commit_c... sync/engine/build_commit_command.cc:25: //Todo(vishwath) Remove this include after node positions have TODO(vishwath): http://codereview.chromium.org/10989063/diff/22004/sync/engine/build_commit_c... sync/engine/build_commit_command.cc:26: // shifted to completely uing Ordinals. uing -> using http://codereview.chromium.org/10989063/diff/22004/sync/engine/build_commit_c... sync/engine/build_commit_command.cc:27: // See crbug.com/145412 prepend http:// to URL (to make it clickable) also add a space and a period http://codereview.chromium.org/10989063/diff/22004/sync/engine/process_commit... File sync/engine/process_commit_response_command.cc (right): http://codereview.chromium.org/10989063/diff/22004/sync/engine/process_commit... sync/engine/process_commit_response_command.cc:25: //TODO(vishwath) Remove this include after node positions have see comment in other file http://codereview.chromium.org/10989063/diff/22004/sync/engine/process_commit... sync/engine/process_commit_response_command.cc:26: // shifted to completely uing Ordinals. uing -> using http://codereview.chromium.org/10989063/diff/22004/sync/engine/process_update... File sync/engine/process_updates_command.cc (right): http://codereview.chromium.org/10989063/diff/22004/sync/engine/process_update... sync/engine/process_updates_command.cc:22: // Temporary include to be removed after the int64->ordinal use comment from other files? http://codereview.chromium.org/10989063/diff/22004/sync/engine/syncer.cc File sync/engine/syncer.cc (right): http://codereview.chromium.org/10989063/diff/22004/sync/engine/syncer.cc#newc... sync/engine/syncer.cc:30: //TODO(vishwath) Remove this include after node positions have same http://codereview.chromium.org/10989063/diff/22004/sync/engine/syncer_util.cc File sync/engine/syncer_util.cc (right): http://codereview.chromium.org/10989063/diff/22004/sync/engine/syncer_util.cc... sync/engine/syncer_util.cc:32: //TODO(vishwath) Remove this include after node positions have same http://codereview.chromium.org/10989063/diff/22004/sync/internal_api/public/b... File sync/internal_api/public/base/ordinal.h (right): http://codereview.chromium.org/10989063/diff/22004/sync/internal_api/public/b... sync/internal_api/public/base/ordinal.h:81: // Creates a valid initial Ordinal. hmm why is this necessary? Why not CreateInitialOrdinal()? http://codereview.chromium.org/10989063/diff/22004/sync/syncable/directory.cc File sync/syncable/directory.cc (right): http://codereview.chromium.org/10989063/diff/22004/sync/syncable/directory.cc... sync/syncable/directory.cc:54: NodeOrdinal a_position = a->ref(SERVER_ORDINAL_IN_PARENT); use const ref http://codereview.chromium.org/10989063/diff/22004/sync/syncable/directory.cc... sync/syncable/directory.cc:55: NodeOrdinal b_position = b->ref(SERVER_ORDINAL_IN_PARENT); here too http://codereview.chromium.org/10989063/diff/22004/sync/syncable/directory_ba... File sync/syncable/directory_backing_store.cc (right): http://codereview.chromium.org/10989063/diff/22004/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:110: kernel->mutable_ref(static_cast<OrdinalField>(i)) = NodeOrdinal(temp); how should we handle invalid NodeOrdinals? DCHECK/CHECK/unrecoverable error, create brand new ordinal, something else? ideas, vishwash / richard? http://codereview.chromium.org/10989063/diff/22004/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:523: for (i = BEGIN_FIELDS; i < ORDINAL_FIELDS_END; ++i) { can you use FIELD_COUNT instead of ORDINAL_FIELDS_END? (maybe even define FIELDS_END = FIELD_COUNT) http://codereview.chromium.org/10989063/diff/22004/sync/syncable/directory_ba... File sync/syncable/directory_backing_store_unittest.cc (right): http://codereview.chromium.org/10989063/diff/22004/sync/syncable/directory_ba... sync/syncable/directory_backing_store_unittest.cc:2154: //ASSERT_TRUE(dbs->Load(&entry_bucket, &load_info)); Richard, you have any idea how to solve this? http://codereview.chromium.org/10989063/diff/22004/sync/syncable/entry.cc File sync/syncable/entry.cc (right): http://codereview.chromium.org/10989063/diff/22004/sync/syncable/entry.cc#new... sync/syncable/entry.cc:131: << net::EscapePath( hunh, just noticed this. this is a weird escaping mechanism. Can you use base/json/string_escape.h instead? http://codereview.chromium.org/10989063/diff/22004/sync/syncable/entry.cc#new... sync/syncable/entry.cc:136: os << g_metas_columns[i].name << ": " can you use ToDebugString() instead? http://codereview.chromium.org/10989063/diff/22004/sync/syncable/entry_kernel.h File sync/syncable/entry_kernel.h (right): http://codereview.chromium.org/10989063/diff/22004/sync/syncable/entry_kernel... sync/syncable/entry_kernel.h:172: PROTO_FIELDS_COUNT = PROTO_FIELDS_END - PROTO_FIELDS_BEGIN move this line right above ORDINAL_FIELDS_BEGIN = PROTO_FIELDS_END http://codereview.chromium.org/10989063/diff/22004/sync/syncable/syncable_col... File sync/syncable/syncable_columns.h (right): http://codereview.chromium.org/10989063/diff/22004/sync/syncable/syncable_col... sync/syncable/syncable_columns.h:56: // Blobs (serialized protos). no period
I just skimmed through this responded to the comments that seemed the most interesting. I'll do a more thorough review tomorrow. http://codereview.chromium.org/10989063/diff/22004/sync/syncable/directory_ba... File sync/syncable/directory_backing_store.cc (right): http://codereview.chromium.org/10989063/diff/22004/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:110: kernel->mutable_ref(static_cast<OrdinalField>(i)) = NodeOrdinal(temp); On 2012/10/05 00:57:59, akalin wrote: > how should we handle invalid NodeOrdinals? DCHECK/CHECK/unrecoverable error, > create brand new ordinal, something else? ideas, vishwash / richard? One option would be to fail to load the directory and declare it corrupt. This would lead to it being deleted and the user's sync data re-downloaded. I think that Ordinal's methods do a pretty good job of maintaining invariants. If we trust that code, then we can assume that invalid ordinals can't/won't be generated by Chrome, so it's valid to assume something outside chrome caused the issue. http://codereview.chromium.org/10989063/diff/22004/sync/syncable/directory_ba... File sync/syncable/directory_backing_store_unittest.cc (right): http://codereview.chromium.org/10989063/diff/22004/sync/syncable/directory_ba... sync/syncable/directory_backing_store_unittest.cc:2154: //ASSERT_TRUE(dbs->Load(&entry_bucket, &load_info)); On 2012/10/05 00:57:59, akalin wrote: > Richard, you have any idea how to solve this? Some quick googling suggests that calling Reset() on any open statements that reference the table might fix the problem.
I can make those changes, it seems like the biggest issues are the table locking and handling invalid ordinals. I did see the Reset() option being touted. Others also suggested closing and reopening the connection itself as a last resort. For the invalid ordinals, I'm on board with forcing the re-download of the directory. Fred, does that seem like a good solution to you too? I can implement that. On Thu, Oct 4, 2012 at 6:10 PM, <rlarocque@chromium.org> wrote: > I just skimmed through this responded to the comments that seemed the most > interesting. I'll do a more thorough review tomorrow. > > > > http://codereview.chromium.**org/10989063/diff/22004/sync/** > syncable/directory_backing_**store.cc<http://codereview.chromium.org/10989063/diff/22004/sync/syncable/directory_backing_store.cc> > File sync/syncable/directory_**backing_store.cc (right): > > http://codereview.chromium.**org/10989063/diff/22004/sync/** > syncable/directory_backing_**store.cc#newcode110<http://codereview.chromium.org/10989063/diff/22004/sync/syncable/directory_backing_store.cc#newcode110> > sync/syncable/directory_**backing_store.cc:110: > kernel->mutable_ref(static_**cast<OrdinalField>(i)) = NodeOrdinal(temp); > On 2012/10/05 00:57:59, akalin wrote: > >> how should we handle invalid NodeOrdinals? DCHECK/CHECK/unrecoverable >> > error, > >> create brand new ordinal, something else? ideas, vishwash / richard? >> > > One option would be to fail to load the directory and declare it > corrupt. This would lead to it being deleted and the user's sync data > re-downloaded. > > I think that Ordinal's methods do a pretty good job of maintaining > invariants. If we trust that code, then we can assume that invalid > ordinals can't/won't be generated by Chrome, so it's valid to assume > something outside chrome caused the issue. > > > http://codereview.chromium.**org/10989063/diff/22004/sync/** > syncable/directory_backing_**store_unittest.cc<http://codereview.chromium.org/10989063/diff/22004/sync/syncable/directory_backing_store_unittest.cc> > File sync/syncable/directory_**backing_store_unittest.cc (right): > > http://codereview.chromium.**org/10989063/diff/22004/sync/** > syncable/directory_backing_**store_unittest.cc#newcode2154<http://codereview.chromium.org/10989063/diff/22004/sync/syncable/directory_backing_store_unittest.cc#newcode2154> > sync/syncable/directory_**backing_store_unittest.cc:**2154: > //ASSERT_TRUE(dbs->Load(&**entry_bucket, &load_info)); > On 2012/10/05 00:57:59, akalin wrote: > >> Richard, you have any idea how to solve this? >> > > Some quick googling suggests that calling Reset() on any open statements > that reference the table might fix the problem. > > http://codereview.chromium.**org/10989063/<http://codereview.chromium.org/109... >
[On Thu, Oct 4, 2012 at 6:39 PM, Vishwath Mohan <vishwath@google.com> wrote: > I can make those changes, it seems like the biggest issues are the table > locking and handling invalid ordinals. I did see the Reset() option being > touted. Others also suggested closing and reopening the connection itself as > a last resort. > > For the invalid ordinals, I'm on board with forcing the re-download of the > directory. Fred, does that seem like a good solution to you too? I can > implement that. Sounds good to me! Add a test for it if possible.
The test I added takes the lazy route of constructing a v80 DB and migrating over to v81 (it seemed easier than creating valid ordinal positions manually for each entry), but that can be changed if it isn't acceptable. https://codereview.chromium.org/10989063/diff/22004/sync/engine/build_commit_... File sync/engine/build_commit_command.cc (right): https://codereview.chromium.org/10989063/diff/22004/sync/engine/build_commit_... sync/engine/build_commit_command.cc:25: //Todo(vishwath) Remove this include after node positions have On 2012/10/05 00:57:59, akalin wrote: > TODO(vishwath): Done. https://codereview.chromium.org/10989063/diff/22004/sync/engine/build_commit_... sync/engine/build_commit_command.cc:26: // shifted to completely uing Ordinals. On 2012/10/05 00:57:59, akalin wrote: > uing -> using Done. https://codereview.chromium.org/10989063/diff/22004/sync/engine/build_commit_... sync/engine/build_commit_command.cc:27: // See crbug.com/145412 On 2012/10/05 00:57:59, akalin wrote: > prepend http:// to URL (to make it clickable) > > also add a space and a period Done. https://codereview.chromium.org/10989063/diff/22004/sync/engine/process_commi... File sync/engine/process_commit_response_command.cc (right): https://codereview.chromium.org/10989063/diff/22004/sync/engine/process_commi... sync/engine/process_commit_response_command.cc:25: //TODO(vishwath) Remove this include after node positions have On 2012/10/05 00:57:59, akalin wrote: > see comment in other file Done. https://codereview.chromium.org/10989063/diff/22004/sync/engine/process_commi... sync/engine/process_commit_response_command.cc:26: // shifted to completely uing Ordinals. On 2012/10/05 00:57:59, akalin wrote: > uing -> using Done. https://codereview.chromium.org/10989063/diff/22004/sync/engine/process_updat... File sync/engine/process_updates_command.cc (right): https://codereview.chromium.org/10989063/diff/22004/sync/engine/process_updat... sync/engine/process_updates_command.cc:22: // Temporary include to be removed after the int64->ordinal On 2012/10/05 00:57:59, akalin wrote: > use comment from other files? Done. https://codereview.chromium.org/10989063/diff/22004/sync/engine/syncer.cc File sync/engine/syncer.cc (right): https://codereview.chromium.org/10989063/diff/22004/sync/engine/syncer.cc#new... sync/engine/syncer.cc:30: //TODO(vishwath) Remove this include after node positions have On 2012/10/05 00:57:59, akalin wrote: > same Done. https://codereview.chromium.org/10989063/diff/22004/sync/internal_api/public/... File sync/internal_api/public/base/ordinal.h (right): https://codereview.chromium.org/10989063/diff/22004/sync/internal_api/public/... sync/internal_api/public/base/ordinal.h:81: // Creates a valid initial Ordinal. There are multiple places where new entry kernels are created and then inserted into directory indices. This creates a problem when the SERVER_ORDINAL field is later queried since it contains an invalid ordinal. I could just add a call to CreateInitialOrdinal() in the entry kernel constructor instead. Do we need the default constructor to create invalid ordinals though? On 2012/10/05 00:57:59, akalin wrote: > hmm why is this necessary? Why not CreateInitialOrdinal()? https://codereview.chromium.org/10989063/diff/22004/sync/syncable/directory.cc File sync/syncable/directory.cc (right): https://codereview.chromium.org/10989063/diff/22004/sync/syncable/directory.c... sync/syncable/directory.cc:54: NodeOrdinal a_position = a->ref(SERVER_ORDINAL_IN_PARENT); On 2012/10/05 00:57:59, akalin wrote: > use const ref Done. https://codereview.chromium.org/10989063/diff/22004/sync/syncable/directory.c... sync/syncable/directory.cc:55: NodeOrdinal b_position = b->ref(SERVER_ORDINAL_IN_PARENT); On 2012/10/05 00:57:59, akalin wrote: > here too Done. https://codereview.chromium.org/10989063/diff/22004/sync/syncable/directory_b... File sync/syncable/directory_backing_store.cc (right): https://codereview.chromium.org/10989063/diff/22004/sync/syncable/directory_b... sync/syncable/directory_backing_store.cc:110: kernel->mutable_ref(static_cast<OrdinalField>(i)) = NodeOrdinal(temp); On 2012/10/05 01:10:50, rlarocque wrote: > On 2012/10/05 00:57:59, akalin wrote: > > how should we handle invalid NodeOrdinals? DCHECK/CHECK/unrecoverable error, > > create brand new ordinal, something else? ideas, vishwash / richard? > > One option would be to fail to load the directory and declare it corrupt. This > would lead to it being deleted and the user's sync data re-downloaded. > > I think that Ordinal's methods do a pretty good job of maintaining invariants. > If we trust that code, then we can assume that invalid ordinals can't/won't be > generated by Chrome, so it's valid to assume something outside chrome caused the > issue. Done. https://codereview.chromium.org/10989063/diff/22004/sync/syncable/directory_b... sync/syncable/directory_backing_store.cc:523: for (i = BEGIN_FIELDS; i < ORDINAL_FIELDS_END; ++i) { Called it END_FIELDS to match with BEGIN_FIELDS, but essentially done. On 2012/10/05 00:57:59, akalin wrote: > can you use FIELD_COUNT instead of ORDINAL_FIELDS_END? > > (maybe even define FIELDS_END = FIELD_COUNT) Done. https://codereview.chromium.org/10989063/diff/22004/sync/syncable/directory_b... File sync/syncable/directory_backing_store_unittest.cc (right): https://codereview.chromium.org/10989063/diff/22004/sync/syncable/directory_b... sync/syncable/directory_backing_store_unittest.cc:2154: //ASSERT_TRUE(dbs->Load(&entry_bucket, &load_info)); I had to call Clear() before Load(), embarrassingly easy. On 2012/10/05 01:10:50, rlarocque wrote: > On 2012/10/05 00:57:59, akalin wrote: > > Richard, you have any idea how to solve this? > > Some quick googling suggests that calling Reset() on any open statements that > reference the table might fix the problem.
https://codereview.chromium.org/10989063/diff/22004/sync/internal_api/public/... File sync/internal_api/public/base/ordinal.h (right): https://codereview.chromium.org/10989063/diff/22004/sync/internal_api/public/... sync/internal_api/public/base/ordinal.h:81: // Creates a valid initial Ordinal. On 2012/10/05 18:34:49, vishwath wrote: > There are multiple places where new entry kernels are created and then inserted > into directory indices. This creates a problem when the SERVER_ORDINAL field is > later queried since it contains an invalid ordinal. I could just add a call to > CreateInitialOrdinal() in the entry kernel constructor instead. Do we need the > default constructor to create invalid ordinals though? > > On 2012/10/05 00:57:59, akalin wrote: > > hmm why is this necessary? Why not CreateInitialOrdinal()? > I think the trend is to initialize those values in the MutableEntry's constructor or init function. The default invalid ordinals are a safety to ensure we don't accidentally use ordinals that we intended to initialize, but didn't. https://codereview.chromium.org/10989063/diff/22004/sync/syncable/directory_b... File sync/syncable/directory_backing_store.cc (right): https://codereview.chromium.org/10989063/diff/22004/sync/syncable/directory_b... sync/syncable/directory_backing_store.cc:1019: std::string ordinal = Int64ToNodeOrdinal(position).ToInternalValue(); I think you can make this string a const ref. https://codereview.chromium.org/10989063/diff/22004/sync/syncable/entry_kernel.h File sync/syncable/entry_kernel.h (right): https://codereview.chromium.org/10989063/diff/22004/sync/syncable/entry_kerne... sync/syncable/entry_kernel.h:51: nit: Might as well delete this line, too, to keep the spacing consistent. https://codereview.chromium.org/10989063/diff/22004/sync/syncable/mutable_ent... File sync/syncable/mutable_entry.cc (right): https://codereview.chromium.org/10989063/diff/22004/sync/syncable/mutable_ent... sync/syncable/mutable_entry.cc:151: lock, kernel_, dir()->kernel_->parent_id_child_index); nit: This indent is off by one. I think you should indent four spaces from the last line. https://codereview.chromium.org/10989063/diff/26003/sync/syncable/mutable_ent... File sync/syncable/mutable_entry.cc (right): https://codereview.chromium.org/10989063/diff/26003/sync/syncable/mutable_ent... sync/syncable/mutable_entry.cc:29: void MutableEntry::Init(WriteTransaction* trans, const Id& parent_id, If we decide that the NodeOrdinal's default constructor should default to an invalid value, then SERVER_ORDINAL_IN_PARENT should be initialized here.
https://codereview.chromium.org/10989063/diff/22004/sync/internal_api/public/... File sync/internal_api/public/base/ordinal.h (right): https://codereview.chromium.org/10989063/diff/22004/sync/internal_api/public/... sync/internal_api/public/base/ordinal.h:81: // Creates a valid initial Ordinal. Entrykernels are also created in directory_backing_store for instance (in the UnpackEntry() method). I could ensure that all instantiations of an entrykernel are followed by setting the server_ordinal_field to a valid value. Does that seem like a better option? On 2012/10/05 19:02:55, rlarocque wrote: > On 2012/10/05 18:34:49, vishwath wrote: > > There are multiple places where new entry kernels are created and then > inserted > > into directory indices. This creates a problem when the SERVER_ORDINAL field > is > > later queried since it contains an invalid ordinal. I could just add a call to > > CreateInitialOrdinal() in the entry kernel constructor instead. Do we need the > > default constructor to create invalid ordinals though? > > > > On 2012/10/05 00:57:59, akalin wrote: > > > hmm why is this necessary? Why not CreateInitialOrdinal()? > > > > I think the trend is to initialize those values in the MutableEntry's > constructor or init function. > > The default invalid ordinals are a safety to ensure we don't accidentally use > ordinals that we intended to initialize, but didn't. https://codereview.chromium.org/10989063/diff/22004/sync/syncable/directory_b... File sync/syncable/directory_backing_store.cc (right): https://codereview.chromium.org/10989063/diff/22004/sync/syncable/directory_b... sync/syncable/directory_backing_store.cc:1019: std::string ordinal = Int64ToNodeOrdinal(position).ToInternalValue(); On 2012/10/05 19:02:55, rlarocque wrote: > I think you can make this string a const ref. Done. https://codereview.chromium.org/10989063/diff/22004/sync/syncable/entry_kernel.h File sync/syncable/entry_kernel.h (right): https://codereview.chromium.org/10989063/diff/22004/sync/syncable/entry_kerne... sync/syncable/entry_kernel.h:51: On 2012/10/05 19:02:55, rlarocque wrote: > nit: Might as well delete this line, too, to keep the spacing consistent. Done. https://codereview.chromium.org/10989063/diff/22004/sync/syncable/entry_kerne... sync/syncable/entry_kernel.h:172: PROTO_FIELDS_COUNT = PROTO_FIELDS_END - PROTO_FIELDS_BEGIN On 2012/10/05 00:57:59, akalin wrote: > move this line right above > > ORDINAL_FIELDS_BEGIN = PROTO_FIELDS_END Done.
https://codereview.chromium.org/10989063/diff/22004/sync/internal_api/public/... File sync/internal_api/public/base/ordinal.h (right): https://codereview.chromium.org/10989063/diff/22004/sync/internal_api/public/... sync/internal_api/public/base/ordinal.h:81: // Creates a valid initial Ordinal. On 2012/10/05 21:05:48, vishwath wrote: > Entrykernels are also created in directory_backing_store for instance (in the > UnpackEntry() method). I could ensure that all instantiations of an entrykernel > are followed by setting the server_ordinal_field to a valid value. Does that > seem like a better option? Yeah. Ideally, EntryKernels would have to be constructed with a valid ordinal.
https://codereview.chromium.org/10989063/diff/22004/sync/internal_api/public/... File sync/internal_api/public/base/ordinal.h (right): https://codereview.chromium.org/10989063/diff/22004/sync/internal_api/public/... sync/internal_api/public/base/ordinal.h:81: // Creates a valid initial Ordinal. On 2012/10/05 21:20:15, akalin wrote: > On 2012/10/05 21:05:48, vishwath wrote: > > Entrykernels are also created in directory_backing_store for instance (in the > > UnpackEntry() method). I could ensure that all instantiations of an > entrykernel > > are followed by setting the server_ordinal_field to a valid value. Does that > > seem like a better option? > > Yeah. Ideally, EntryKernels would have to be constructed with a valid ordinal. I think I agree, but I just want to clarify. EntryKernels have a lot more in common with C structs than C++ classes. It currently has a single constructor that takes no parameters. If we require that the caller provide a valid server_ordinal, it would be the first field in the EntryKernel to impose such a requirement. I think it makes sense to require the context that creates the EntryKernel to properly initialize its fields. The Ordinal class has plenty of DCHECKs that would help ensure that no one accesses the invalid, default-constructed ordinals. Is that what you're proposing? If the zero-argument constructor initialized the ordinal to a valid value automatically, then we would no longer be able to distinguish between a default and unintentionally never set value.
The default ctor is back to creating invalid Ordinals now. I've changed it so that the MutableEntry ctors initialize the field instead.
Moar comments http://codereview.chromium.org/10989063/diff/34002/sync/engine/build_commit_c... File sync/engine/build_commit_command.cc (right): http://codereview.chromium.org/10989063/diff/34002/sync/engine/build_commit_c... sync/engine/build_commit_command.cc:25: //TODO(vishwath): Remove this include after node positions have space after // http://codereview.chromium.org/10989063/diff/34002/sync/engine/process_commit... File sync/engine/process_commit_response_command.cc (right): http://codereview.chromium.org/10989063/diff/34002/sync/engine/process_commit... sync/engine/process_commit_response_command.cc:25: //TODO(vishwath): Remove this include after node positions have same http://codereview.chromium.org/10989063/diff/34002/sync/engine/process_update... File sync/engine/process_updates_command.cc (right): http://codereview.chromium.org/10989063/diff/34002/sync/engine/process_update... sync/engine/process_updates_command.cc:22: // Temporary include to be removed after the int64->ordinal make same as other comments? http://codereview.chromium.org/10989063/diff/34002/sync/engine/syncer.cc File sync/engine/syncer.cc (right): http://codereview.chromium.org/10989063/diff/34002/sync/engine/syncer.cc#newc... sync/engine/syncer.cc:30: //TODO(vishwath): Remove this include after node positions have space after // http://codereview.chromium.org/10989063/diff/34002/sync/engine/syncer_util.cc File sync/engine/syncer_util.cc (right): http://codereview.chromium.org/10989063/diff/34002/sync/engine/syncer_util.cc... sync/engine/syncer_util.cc:32: //TODO(vishwath): Remove this include after node positions have space after // http://codereview.chromium.org/10989063/diff/34002/sync/internal_api/public/b... File sync/internal_api/public/base/ordinal.h (right): http://codereview.chromium.org/10989063/diff/34002/sync/internal_api/public/b... sync/internal_api/public/base/ordinal.h:81: // Creates a valid initial Ordinal. revert this http://codereview.chromium.org/10989063/diff/34002/sync/internal_api/public/b... sync/internal_api/public/base/ordinal.h:237: //template <typename Traits> remove this http://codereview.chromium.org/10989063/diff/34002/sync/internal_api/public/b... File sync/internal_api/public/base/ordinal_unittest.cc (right): http://codereview.chromium.org/10989063/diff/34002/sync/internal_api/public/b... sync/internal_api/public/base/ordinal_unittest.cc:134: const TestOrdinal invalid_ordinal(""); revert http://codereview.chromium.org/10989063/diff/34002/sync/internal_api/sync_man... File sync/internal_api/sync_manager_impl_unittest.cc (right): http://codereview.chromium.org/10989063/diff/34002/sync/internal_api/sync_man... sync/internal_api/sync_manager_impl_unittest.cc:28: #include "sync/internal_api/public/base/node_ordinal.h" still needed? http://codereview.chromium.org/10989063/diff/34002/sync/syncable/directory_ba... File sync/syncable/directory_backing_store.cc (right): http://codereview.chromium.org/10989063/diff/34002/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:117: DVLOG(1) <<"Unpacked invalid ordinal. Signaling that the DB is corrupt"; space after << http://codereview.chromium.org/10989063/diff/34002/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:118: return NULL; you're leaking kernel here! Looks like a good time to make kernel a scoped_ptr<EntryKernel>. May as well return a scoped_ptr, also. http://codereview.chromium.org/10989063/diff/34002/sync/syncable/directory_ba... sync/syncable/directory_backing_store.cc:458: EntryKernel *kernel = UnpackEntry(&s); ' *' -> '* ', but this should be a scoped_ptr anyway http://codereview.chromium.org/10989063/diff/34002/sync/syncable/directory_ba... File sync/syncable/directory_backing_store_unittest.cc (right): http://codereview.chromium.org/10989063/diff/34002/sync/syncable/directory_ba... sync/syncable/directory_backing_store_unittest.cc:1640: "INSERT INTO 'share_version' VALUES('nick@chromium.org',79);" nice catch http://codereview.chromium.org/10989063/diff/34002/sync/syncable/directory_ba... sync/syncable/directory_backing_store_unittest.cc:2172: remove this extra line? http://codereview.chromium.org/10989063/diff/34002/sync/syncable/directory_ba... sync/syncable/directory_backing_store_unittest.cc:2217: scoped_ptr<TestDirectoryBackingStore> dbs( actually, can you make a SetUpVersion81Database? Sorry, it's a pain, but you'll make the life of the person who has to do the next migration easier. :) http://codereview.chromium.org/10989063/diff/34002/sync/syncable/directory_ba... sync/syncable/directory_backing_store_unittest.cc:2222: //Insert row with bad ordinal. space after // http://codereview.chromium.org/10989063/diff/34002/sync/syncable/directory_ba... sync/syncable/directory_backing_store_unittest.cc:2232: //Trying to unpack this entry should signal that the DB is corrupted. space after // http://codereview.chromium.org/10989063/diff/34002/sync/syncable/entry.cc File sync/syncable/entry.cc (right): http://codereview.chromium.org/10989063/diff/34002/sync/syncable/entry.cc#new... sync/syncable/entry.cc:132: kernel->ref(static_cast<ProtoField>(i)).SerializeAsString()) did you miss my comments here re. having printable strings? http://codereview.chromium.org/10989063/diff/34002/sync/syncable/entry.cc#new... sync/syncable/entry.cc:137: << kernel->ref(static_cast<OrdinalField>(i)).ToInternalValue() and here? http://codereview.chromium.org/10989063/diff/34002/sync/syncable/mutable_entr... File sync/syncable/mutable_entry.cc (right): http://codereview.chromium.org/10989063/diff/34002/sync/syncable/mutable_entr... sync/syncable/mutable_entry.cc:148: DCHECK(kernel_); DCHECK here that |value| is a valid ordinal http://codereview.chromium.org/10989063/diff/34002/sync/syncable/syncable_col... File sync/syncable/syncable_columns.h (right): http://codereview.chromium.org/10989063/diff/34002/sync/syncable/syncable_col... sync/syncable/syncable_columns.h:61: // Blobs (ordinals) period after (ordinals) http://codereview.chromium.org/10989063/diff/34002/sync/syncable/syncable_uni... File sync/syncable/syncable_unittest.cc (right): http://codereview.chromium.org/10989063/diff/34002/sync/syncable/syncable_uni... sync/syncable/syncable_unittest.cc:20: #include "sync/internal_api/public/base/ordinal.h" no need for ordinal.h http://codereview.chromium.org/10989063/diff/34002/sync/syncable/syncable_uni... sync/syncable/syncable_unittest.cc:1371: TEST_F(SyncableDirectoryTest, OrdinalHandlesNullValues) { maybe a better name would be OrdinalWithNullSurvivesSaveAndReload (or something) http://codereview.chromium.org/10989063/diff/34002/sync/syncable/syncable_uni... sync/syncable/syncable_unittest.cc:1375: std::string null_str(null_cstr, 9); 9 -> arraysize(null_cstr) (include basictypes.h for arraysize)
> EntryKernels have a lot more in common with C structs than C++ classes. It > currently has a single constructor that takes no parameters. If we require that > the caller provide a valid server_ordinal, it would be the first field in the > EntryKernel to impose such a requirement. > > I think it makes sense to require the context that creates the EntryKernel to > properly initialize its fields. The Ordinal class has plenty of DCHECKs that > would help ensure that no one accesses the invalid, default-constructed > ordinals. Is that what you're proposing? > > If the zero-argument constructor initialized the ordinal to a valid value > automatically, then we would no longer be able to distinguish between a default > and unintentionally never set value. Yeah, that's true. This would be the first field. But I think this is the first field type that can be invalid, right? We can also hide the default constructor for EntryKernel, right?
On 2012/10/05 22:42:05, akalin wrote: > > EntryKernels have a lot more in common with C structs than C++ classes. It > > currently has a single constructor that takes no parameters. If we require > that > > the caller provide a valid server_ordinal, it would be the first field in the > > EntryKernel to impose such a requirement. > > > > I think it makes sense to require the context that creates the EntryKernel to > > properly initialize its fields. The Ordinal class has plenty of DCHECKs that > > would help ensure that no one accesses the invalid, default-constructed > > ordinals. Is that what you're proposing? > > > > If the zero-argument constructor initialized the ordinal to a valid value > > automatically, then we would no longer be able to distinguish between a > default > > and unintentionally never set value. > > Yeah, that's true. This would be the first field. But I think this is the > first field type that can be invalid, right? There are a few shades of grey there, but I'd say the answer is no. The worst offender is the META_HANDLE field, which is supposed to be unique, but the constructor always leaves it as 0. I'd say that's invalid. The protos are initially an empty string, which is invalid for most (but admittedly not all) EntryKernels. All the ID fields start off with the root/null value "r". The time fields are set to 0, which isn't wrong, but not exactly right, either. > We can also hide the default constructor for EntryKernel, right? We could, but that's not what's currently done. I'm not against moving towards a more C++-like EntryKernel. In fact, I'd love to spend a week rewriting the entire syncable layer to make proper use of C++-style objects. For this CL, though, I think we should stick with the current approach. It will be less confusing if we keep things consistent.
On 2012/10/05 23:02:07, rlarocque wrote: > > I'm not against moving towards a more C++-like EntryKernel. In fact, I'd love > to spend a week rewriting the entire syncable layer to make proper use of > C++-style objects. For this CL, though, I think we should stick with the > current approach. It will be less confusing if we keep things consistent. Okay, sounds good.
https://codereview.chromium.org/10989063/diff/34002/sync/engine/build_commit_... File sync/engine/build_commit_command.cc (right): https://codereview.chromium.org/10989063/diff/34002/sync/engine/build_commit_... sync/engine/build_commit_command.cc:25: //TODO(vishwath): Remove this include after node positions have On 2012/10/05 22:40:27, akalin wrote: > space after // Done. https://codereview.chromium.org/10989063/diff/34002/sync/engine/process_commi... File sync/engine/process_commit_response_command.cc (right): https://codereview.chromium.org/10989063/diff/34002/sync/engine/process_commi... sync/engine/process_commit_response_command.cc:25: //TODO(vishwath): Remove this include after node positions have On 2012/10/05 22:40:27, akalin wrote: > same Done. https://codereview.chromium.org/10989063/diff/34002/sync/engine/process_updat... File sync/engine/process_updates_command.cc (right): https://codereview.chromium.org/10989063/diff/34002/sync/engine/process_updat... sync/engine/process_updates_command.cc:22: // Temporary include to be removed after the int64->ordinal On 2012/10/05 22:40:27, akalin wrote: > make same as other comments? Done. https://codereview.chromium.org/10989063/diff/34002/sync/engine/syncer.cc File sync/engine/syncer.cc (right): https://codereview.chromium.org/10989063/diff/34002/sync/engine/syncer.cc#new... sync/engine/syncer.cc:30: //TODO(vishwath): Remove this include after node positions have On 2012/10/05 22:40:27, akalin wrote: > space after // Done. https://codereview.chromium.org/10989063/diff/34002/sync/internal_api/public/... File sync/internal_api/public/base/ordinal.h (right): https://codereview.chromium.org/10989063/diff/34002/sync/internal_api/public/... sync/internal_api/public/base/ordinal.h:81: // Creates a valid initial Ordinal. On 2012/10/05 22:40:27, akalin wrote: > revert this Done. https://codereview.chromium.org/10989063/diff/34002/sync/internal_api/public/... sync/internal_api/public/base/ordinal.h:237: //template <typename Traits> On 2012/10/05 22:40:27, akalin wrote: > remove this Done. https://codereview.chromium.org/10989063/diff/34002/sync/internal_api/public/... File sync/internal_api/public/base/ordinal_unittest.cc (right): https://codereview.chromium.org/10989063/diff/34002/sync/internal_api/public/... sync/internal_api/public/base/ordinal_unittest.cc:134: const TestOrdinal invalid_ordinal(""); On 2012/10/05 22:40:27, akalin wrote: > revert Done. https://codereview.chromium.org/10989063/diff/34002/sync/internal_api/sync_ma... File sync/internal_api/sync_manager_impl_unittest.cc (right): https://codereview.chromium.org/10989063/diff/34002/sync/internal_api/sync_ma... sync/internal_api/sync_manager_impl_unittest.cc:28: #include "sync/internal_api/public/base/node_ordinal.h" On 2012/10/05 22:40:27, akalin wrote: > still needed? Done. https://codereview.chromium.org/10989063/diff/34002/sync/syncable/directory_b... File sync/syncable/directory_backing_store.cc (right): https://codereview.chromium.org/10989063/diff/34002/sync/syncable/directory_b... sync/syncable/directory_backing_store.cc:118: return NULL; On 2012/10/05 22:40:27, akalin wrote: > you're leaking kernel here! Looks like a good time to make kernel a > scoped_ptr<EntryKernel>. May as well return a scoped_ptr, also. Done. https://codereview.chromium.org/10989063/diff/34002/sync/syncable/directory_b... sync/syncable/directory_backing_store.cc:458: EntryKernel *kernel = UnpackEntry(&s); On 2012/10/05 22:40:27, akalin wrote: > ' *' -> '* ', but this should be a scoped_ptr anyway Done. https://codereview.chromium.org/10989063/diff/34002/sync/syncable/directory_b... File sync/syncable/directory_backing_store_unittest.cc (right): https://codereview.chromium.org/10989063/diff/34002/sync/syncable/directory_b... sync/syncable/directory_backing_store_unittest.cc:2172: On 2012/10/05 22:40:27, akalin wrote: > remove this extra line? Done. https://codereview.chromium.org/10989063/diff/34002/sync/syncable/directory_b... sync/syncable/directory_backing_store_unittest.cc:2217: scoped_ptr<TestDirectoryBackingStore> dbs( On 2012/10/05 22:40:27, akalin wrote: > actually, can you make a SetUpVersion81Database? Sorry, it's a pain, but you'll > make the life of the person who has to do the next migration easier. :) Done. https://codereview.chromium.org/10989063/diff/34002/sync/syncable/directory_b... sync/syncable/directory_backing_store_unittest.cc:2222: //Insert row with bad ordinal. On 2012/10/05 22:40:27, akalin wrote: > space after // Done. https://codereview.chromium.org/10989063/diff/34002/sync/syncable/directory_b... sync/syncable/directory_backing_store_unittest.cc:2232: //Trying to unpack this entry should signal that the DB is corrupted. On 2012/10/05 22:40:27, akalin wrote: > space after // Done. https://codereview.chromium.org/10989063/diff/34002/sync/syncable/entry.cc File sync/syncable/entry.cc (right): https://codereview.chromium.org/10989063/diff/34002/sync/syncable/entry.cc#ne... sync/syncable/entry.cc:132: kernel->ref(static_cast<ProtoField>(i)).SerializeAsString()) Yeah, I'm not sure how I missed these two. Done. On 2012/10/05 22:40:27, akalin wrote: > did you miss my comments here re. having printable strings? https://codereview.chromium.org/10989063/diff/34002/sync/syncable/entry.cc#ne... sync/syncable/entry.cc:137: << kernel->ref(static_cast<OrdinalField>(i)).ToInternalValue() On 2012/10/05 22:40:27, akalin wrote: > and here? Done. https://codereview.chromium.org/10989063/diff/34002/sync/syncable/mutable_ent... File sync/syncable/mutable_entry.cc (right): https://codereview.chromium.org/10989063/diff/34002/sync/syncable/mutable_ent... sync/syncable/mutable_entry.cc:148: DCHECK(kernel_); On 2012/10/05 22:40:27, akalin wrote: > DCHECK here that |value| is a valid ordinal Done. https://codereview.chromium.org/10989063/diff/34002/sync/syncable/syncable_co... File sync/syncable/syncable_columns.h (right): https://codereview.chromium.org/10989063/diff/34002/sync/syncable/syncable_co... sync/syncable/syncable_columns.h:61: // Blobs (ordinals) On 2012/10/05 22:40:27, akalin wrote: > period after (ordinals) Done. https://codereview.chromium.org/10989063/diff/34002/sync/syncable/syncable_un... File sync/syncable/syncable_unittest.cc (right): https://codereview.chromium.org/10989063/diff/34002/sync/syncable/syncable_un... sync/syncable/syncable_unittest.cc:20: #include "sync/internal_api/public/base/ordinal.h" On 2012/10/05 22:40:27, akalin wrote: > no need for ordinal.h Done. https://codereview.chromium.org/10989063/diff/34002/sync/syncable/syncable_un... sync/syncable/syncable_unittest.cc:1371: TEST_F(SyncableDirectoryTest, OrdinalHandlesNullValues) { On 2012/10/05 22:40:27, akalin wrote: > maybe a better name would be OrdinalWithNullSurvivesSaveAndReload (or something) Done. https://codereview.chromium.org/10989063/diff/34002/sync/syncable/syncable_un... sync/syncable/syncable_unittest.cc:1375: std::string null_str(null_cstr, 9); On 2012/10/05 22:40:27, akalin wrote: > 9 -> arraysize(null_cstr) > > (include basictypes.h for arraysize) Done.
Some minor comments. Everything else looks OK. https://codereview.chromium.org/10989063/diff/48001/sync/syncable/directory_b... File sync/syncable/directory_backing_store_unittest.cc (right): https://codereview.chromium.org/10989063/diff/48001/sync/syncable/directory_b... sync/syncable/directory_backing_store_unittest.cc:2344: TEST_F(DirectoryBackingStoreTest, InvalidOrdinalTriggersRedownload) { I wouldn't call this "TriggersRedownload". That happens further up the stack, and your test isn't looking at that part of the code. How about "DetectInvalidOrdinal"? https://codereview.chromium.org/10989063/diff/48001/sync/syncable/entry_kernel.h File sync/syncable/entry_kernel.h (right): https://codereview.chromium.org/10989063/diff/48001/sync/syncable/entry_kerne... sync/syncable/entry_kernel.h:154: END_FIELDS = ORDINAL_FIELDS_END, Is it necessary to have both FIELD_COUNT and END_FIELDS? https://codereview.chromium.org/10989063/diff/48001/sync/syncable/syncable_un... File sync/syncable/syncable_unittest.cc (right): https://codereview.chromium.org/10989063/diff/48001/sync/syncable/syncable_un... sync/syncable/syncable_unittest.cc:1760: nit: unnecessary whitespace.
LGTM after richard's and my nits https://codereview.chromium.org/10989063/diff/48001/sync/internal_api/public/... File sync/internal_api/public/base/ordinal.h (right): https://codereview.chromium.org/10989063/diff/48001/sync/internal_api/public/... sync/internal_api/public/base/ordinal.h:81: // Creates an invalid Ordinal revert diff (add the period back in) https://codereview.chromium.org/10989063/diff/48001/sync/syncable/directory_b... File sync/syncable/directory_backing_store.cc (right): https://codereview.chromium.org/10989063/diff/48001/sync/syncable/directory_b... sync/syncable/directory_backing_store.cc:80: // points to a valid row in the metas table. Returns null to indicate that null -> NULL https://codereview.chromium.org/10989063/diff/48001/sync/syncable/directory_b... sync/syncable/directory_backing_store.cc:114: // some external corruption has occurred. Return null to force null -> NULL https://codereview.chromium.org/10989063/diff/48001/sync/syncable/directory_b... sync/syncable/directory_backing_store.cc:346: // Version 81 changes the server_position_in_parent_field from an int64 update comment. Something like: // Version 81 replaces the int64 server_position_in_parent field with a blob server_ordinal_in_parent field. https://codereview.chromium.org/10989063/diff/48001/sync/syncable/directory_b... sync/syncable/directory_backing_store.cc:460: if(kernel == NULL) usual style is "if (!kernel)" (also, space between 'if' and '(') https://codereview.chromium.org/10989063/diff/48001/sync/syncable/directory_b... sync/syncable/directory_backing_store.cc:1112: std::string ord = NodeOrdinal::CreateInitialOrdinal().ToInternalValue(); const std::string (but not const ref) https://codereview.chromium.org/10989063/diff/48001/sync/syncable/directory_b... File sync/syncable/directory_backing_store_unittest.cc (right): https://codereview.chromium.org/10989063/diff/48001/sync/syncable/directory_b... sync/syncable/directory_backing_store_unittest.cc:1829: namespace { newline after namespace { https://codereview.chromium.org/10989063/diff/48001/sync/syncable/directory_b... sync/syncable/directory_backing_store_unittest.cc:1844: 1048576 }; newline before } https://codereview.chromium.org/10989063/diff/48001/sync/syncable/directory_b... sync/syncable/directory_backing_store_unittest.cc:1846: const std::string V81_Ordinal(int n) { no need for const in return value https://codereview.chromium.org/10989063/diff/48001/sync/syncable/directory_b... sync/syncable/directory_backing_store_unittest.cc:1885: std::string insert_stmts[V80_ROW_COUNT] = { can this be "const char* insert_stmts[...]" instead? https://codereview.chromium.org/10989063/diff/48001/sync/syncable/entry.cc File sync/syncable/entry.cc (right): https://codereview.chromium.org/10989063/diff/48001/sync/syncable/entry.cc#ne... sync/syncable/entry.cc:131: << base::GetDoubleQuotedJson( use JsonDoubleQuote with put_in_quotes set to false instead https://codereview.chromium.org/10989063/diff/48001/sync/syncable/mutable_ent... File sync/syncable/mutable_entry.cc (right): https://codereview.chromium.org/10989063/diff/48001/sync/syncable/mutable_ent... sync/syncable/mutable_entry.cc:147: bool MutableEntry::Put(OrdinalField field, const NodeOrdinal& value) { put this in the right place to match order in .h file
LGTM. I'll leave it up to you whether or not you want to reconsider the test database setup. https://codereview.chromium.org/10989063/diff/48003/sync/syncable/directory_b... File sync/syncable/directory_backing_store_unittest.cc (right): https://codereview.chromium.org/10989063/diff/48003/sync/syncable/directory_b... sync/syncable/directory_backing_store_unittest.cc:1854: // Unlike the earlier versions, the rows for version 81 are generated That's not necessary. You could have used escape codes to represent whatever values you like. It might not be worth the effort, though. I'm willing to accept this function as it's currently written.
https://codereview.chromium.org/10989063/diff/48003/sync/syncable/directory_b... File sync/syncable/directory_backing_store_unittest.cc (right): https://codereview.chromium.org/10989063/diff/48003/sync/syncable/directory_b... sync/syncable/directory_backing_store_unittest.cc:1854: // Unlike the earlier versions, the rows for version 81 are generated On 2012/10/08 21:44:59, rlarocque wrote: > That's not necessary. You could have used escape codes to represent whatever > values you like. I'm not sure that's true. Even with escape codes, embedded nulls are a problem.
Yeah, I wasn't sure how to deal with null characters. I think there are ways to do it, but it doesn't seem worth the trade-off in time/effort. Fred, any other comments or feedback about the code? On Mon, Oct 8, 2012 at 2:46 PM, <akalin@chromium.org> wrote: > > https://codereview.chromium.**org/10989063/diff/48003/sync/** > syncable/directory_backing_**store_unittest.cc<https://codereview.chromium.org/10989063/diff/48003/sync/syncable/directory_backing_store_unittest.cc> > File sync/syncable/directory_**backing_store_unittest.cc (right): > > https://codereview.chromium.**org/10989063/diff/48003/sync/** > syncable/directory_backing_**store_unittest.cc#newcode1854<https://codereview.chromium.org/10989063/diff/48003/sync/syncable/directory_backing_store_unittest.cc#newcode1854> > sync/syncable/directory_**backing_store_unittest.cc:**1854: // Unlike the > earlier versions, the rows for version 81 are generated > On 2012/10/08 21:44:59, rlarocque wrote: > >> That's not necessary. You could have used escape codes to represent >> > whatever > >> values you like. >> > > I'm not sure that's true. Even with escape codes, embedded nulls are a > problem. > > https://codereview.chromium.**org/10989063/<https://codereview.chromium.org/1... >
couple more, but LGTM! https://codereview.chromium.org/10989063/diff/48003/sync/engine/syncer.cc File sync/engine/syncer.cc (right): https://codereview.chromium.org/10989063/diff/48003/sync/engine/syncer.cc#new... sync/engine/syncer.cc:30: //TODO (vishwath): Remove this include after node positions have space after //, remove space after TODO https://codereview.chromium.org/10989063/diff/48003/sync/syncable/entry.cc File sync/syncable/entry.cc (right): https://codereview.chromium.org/10989063/diff/48003/sync/syncable/entry.cc#ne... sync/syncable/entry.cc:137: << ", "; can probably just put this on previous line (and maybe on the line before that, if it fits)
On 2012/10/08 21:46:14, akalin wrote: > https://codereview.chromium.org/10989063/diff/48003/sync/syncable/directory_b... > File sync/syncable/directory_backing_store_unittest.cc (right): > > https://codereview.chromium.org/10989063/diff/48003/sync/syncable/directory_b... > sync/syncable/directory_backing_store_unittest.cc:1854: // Unlike the earlier > versions, the rows for version 81 are generated > On 2012/10/08 21:44:59, rlarocque wrote: > > That's not necessary. You could have used escape codes to represent whatever > > values you like. > > I'm not sure that's true. Even with escape codes, embedded nulls are a problem. Only at the C++ layer. I was thinking of using something like sqlite blob literals. http://www.sqlite.org/lang_expr.html (search for 'blob literal')
On Mon, Oct 8, 2012 at 4:21 PM, <rlarocque@chromium.org> wrote: > Only at the C++ layer. I was thinking of using something like sqlite blob > literals. http://www.sqlite.org/lang_expr.html (search for 'blob literal') > > https://codereview.chromium.org/10989063/ I see. Possible follow-up CL?
On 2012/10/08 23:24:36, akalin wrote: > On Mon, Oct 8, 2012 at 4:21 PM, <mailto:rlarocque@chromium.org> wrote: > > Only at the C++ layer. I was thinking of using something like sqlite blob > > literals. http://www.sqlite.org/lang_expr.html (search for 'blob literal') > > > > https://codereview.chromium.org/10989063/ > > I see. Possible follow-up CL? SGTM.
Does this look good to both of you?
LGTM.
lgtm!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vishwath@google.com/10989063/55026
Change committed as 160774
Its been a while since I've worked with manually managed memory, silly mistake on my end. Should be fixed now.
Ignore the last patch, creating a separate issue for it instead. On Tue, Oct 9, 2012 at 11:28 AM, <vishwath@google.com> wrote: > Its been a while since I've worked with manually managed memory, silly > mistake > on my end. Should be fixed now. > > https://codereview.chromium.**org/10989063/<https://codereview.chromium.org/1... > |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
