Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
115243007:
Basic implementation of the Sync C++ fake server (Closed)
|
Created:
7 years ago by pval...(no longer on Chromium) Modified:
6 years, 11 months ago CC:
chromium-reviews, tim+watch_chromium.org, rsimha+watch_chromium.org, haitaol+watch_chromium.org, maniscalco+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionBasic implementation of the Sync C++ fake server
This CL provides just enough functionality so that
PrototypeFakeServerTest will pass.
BUG=323265
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245326
Patch Set 1 : #
Total comments: 57
Patch Set 2 : #Patch Set 3 : #
Total comments: 12
Patch Set 4 : #Patch Set 5 : factory method for UpdateSieve #Patch Set 6 : move files #
Total comments: 2
Patch Set 7 : #Patch Set 8 : rebase #Patch Set 9 : dependencies change #Patch Set 10 : fix constant for clang #Messages
Total messages: 24 (0 generated)
I realize this is poor timing in sending out this CL (I'll be out of office
starting Thursday, December 19), but I figure I should get it out now.
Albert, I'd appreciate your sanity check on this initial fake server
implementation (specifically fake_server.{h,cc}).
Here are my initial comments. I might be misunderstanding your use of tags and ID values. We should talk in person about that. https://codereview.chromium.org/115243007/diff/40001/chrome/browser/sync/test... File chrome/browser/sync/test/integration/prototype_fake_server_test.cc (right): https://codereview.chromium.org/115243007/diff/40001/chrome/browser/sync/test... chrome/browser/sync/test/integration/prototype_fake_server_test.cc:19: // TODO(pvalenzuela): Remove this test when sync_integration_tests is You should reference the crbug for the fake server development in this comment. https://codereview.chromium.org/115243007/diff/40001/chrome/browser/sync/test... File chrome/browser/sync/test/integration/sync_test.cc (right): https://codereview.chromium.org/115243007/diff/40001/chrome/browser/sync/test... chrome/browser/sync/test/integration/sync_test.cc:880: server_type_ = FAKE_SERVER; Can you somehow DCHECK that SetUp() hasn't been called yet? Maybe DCHECK_EQ(SERVER_TYPE_UNDECIDED, server_type_) would do the trick? https://codereview.chromium.org/115243007/diff/40001/chrome/browser/sync/test... File chrome/browser/sync/test/integration/sync_test.h (right): https://codereview.chromium.org/115243007/diff/40001/chrome/browser/sync/test... chrome/browser/sync/test/integration/sync_test.h:69: FAKE_SERVER, // The fake, in-process server (FakeServer) that I'm not sure if FAKE_SERVER is the best name for this. Wouldn't that be an equally appropriate name for the python server? How about IN_PROCESS_SERVER? Maybe someone else on the review has a better idea. https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/public... File sync/internal_api/public/test/fake_server.h (right): https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/public... sync/internal_api/public/test/fake_server.h:19: class FakeServer { General comment: I think you'll find it easier in the long run to keep up with server and client features if you split up functionality for different types. Certain types, like experiments or push messaging, are not like the others. If you continue down the current path, you might one day find your code littered with if (type == PUSH_MESSAGING) or (type == BOOKMARKS) conditions all over the place. If you implement one class (or at least one instance of some common class) per type, you might have an easier time handling each type's special features. You don't need to act on this comment in this CL, or at all. However, if you agree that splitting up functionality per type is more future proof, you'll probably want to start with that design from the start rather than trying to refactor it in later. https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/public... sync/internal_api/public/test/fake_server.h:30: std::string HandleCommand(std::string request, int* error_code, nit: one parameter per line. http://www.chromium.org/developers/coding-style#Code_Formatting Also, it seems a little odd to me that this combines both out parameters and a return value in this way. Normally the pattern would be something like int error = DoFailableAction(&result); where there is some sort of guarantee that result is not modified when the return value indicates an error. https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/public... sync/internal_api/public/test/fake_server.h:37: class UpdateSieve { I see few advantages to using inner-classes in C++. Since you declared it private, I'm guessing your intention is to hide it. If that's the case, you're probably better off declaring it in an anonymous namespace in the .cpp file. https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/public... sync/internal_api/public/test/fake_server.h:62: typedef std::map<int, int64> TypeIdToVersionMap; Since we're in C++ land and you have the classes available, you'd might as well make this a map from ModelType to int64. https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/public... sync/internal_api/public/test/fake_server.h:67: // Maps data type IDs to the latest version seen for that version. "version seen for that version" -> typo? https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/public... sync/internal_api/public/test/fake_server.h:68: TypeIdToVersionMap state_; state_ is not a very informative name. How about: - type_progress_ - type_min_versions_ other suggestions welcome. https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... File sync/internal_api/test/fake_server.cc (right): https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... sync/internal_api/test/fake_server.cc:20: static const std::string kRootParentTag = "0"; I'm not 100% sure about this, but I think this line creates a static initializer. Declaring this as a char[] would be a bit uglier, but would almost certainly prevent the creation of a static initializer. On the other hand, this is test code that only gets compiled into a test binary, so I don't know if we care either way. https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... sync/internal_api/test/fake_server.cc:46: state_[ModelTypeFromInt(TOP_LEVEL_FOLDER)] = min_version_; This looks sketchy to me. TOP_LEVEL_FOLDER is enough of an exception that it might be better keep it out of the map and (if necessary) store its version in a separate member variable. You might need more if statements in ClientWantsItem(), but you would need less special case code here and in UpdateProgressMarkers. This seems less error-prone. FWIW, the client is slightly inconsistent about this, but in general it tries to treat TOP_LEVEL_FOLDER items as having the same type as their children. So the node with SERVER_UNIQUE_TAG of google_chrome_preferences would often be observed as having type PREFERENCES. https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... sync/internal_api/test/fake_server.cc:85: if (state_[it->first] == 0) Could TOP_LEVEL_FOLDER wind up in this list of types? Would that make sense? https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... sync/internal_api/test/fake_server.cc:114: spec.parent_tag = kRootParentTag; It seems you're using tags as IDs? The root node's ID should be "r". https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... sync/internal_api/test/fake_server.cc:135: In certain situations, you should create the mobile bookmarks folder, too. If you see your tests crashing in strange ways on mobile clients, this may be the reason why. If they don't crash, feel free to ignore this comment and implement that feature later. https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... sync/internal_api/test/fake_server.cc:141: for (size_t i = 0; i < permanent_item_specs_.size(); i++) { Why is the creation of permanent items a two-step process? Rather than creating a bunch of PermanentItemSpecs then iterating over them to transform them into SyncEntities, couldn't you just create them as SyncEntities from the start? https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... sync/internal_api/test/fake_server.cc:166: for (size_t i = 0; i < permanent_item_specs_.size(); i++) { nit: You should prefer to use STL iterators rather than indexes where possible. https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... sync/internal_api/test/fake_server.cc:168: entity.set_parent_id_string(permanent_item_specs_[i].tag); This looks like it's redundant. Can't you just call entity.set_parent_id_string(spec.parent_tag) and skip this iteration? https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... sync/internal_api/test/fake_server.cc:170: entity.set_position_in_parent(1337); I think this should be unset on all items that are neither bookmarks nor bookmark folders.
Some comments about fake_server.{cc,h}
https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f...
File sync/internal_api/test/fake_server.cc (right):
https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f...
sync/internal_api/test/fake_server.cc:135:
Specifically, the situation when you need to create the mobile bookmark folder
is when ClientToServerMessage.get_updates.create_mobile_bookmarks_folder ==
true.
https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f...
sync/internal_api/test/fake_server.cc:191:
entity.set_originator_cache_guid(original_entity.originator_cache_guid());
The client should be setting originator_cache_guid, we never modify that field
on the server.
https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f...
sync/internal_api/test/fake_server.cc:192: entity.set_originator_client_item_id(
Same comment about originator_client_item_id.
https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f...
sync/internal_api/test/fake_server.cc:233:
get_updates_response->set_changes_remaining(0);
Will all SyncEntities always fit in a single response? Shouldn't FakeServer
respect GetUpdatesMessage.batch_size?
https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f...
sync/internal_api/test/fake_server.cc:264: if (sending_nigori ||
message.get_updates().need_encryption_key()) {
Note that sending_nigori does not necessarily imply that you should add
encryption_keys to the response. For custom passphrase users, we never send back
encryption keys.
https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... File sync/internal_api/test/fake_server.cc (right): https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... sync/internal_api/test/fake_server.cc:191: entity.set_originator_cache_guid(original_entity.originator_cache_guid()); On 2013/12/18 20:45:30, albertb wrote: > The client should be setting originator_cache_guid, we never modify that field > on the server. Are you sure? I'm pretty sure we're not setting it on the client. "git grep set_originator_cache_guid" turns up nothing but unit tests on the client.
https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... File sync/internal_api/test/fake_server.cc (right): https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... sync/internal_api/test/fake_server.cc:191: entity.set_originator_cache_guid(original_entity.originator_cache_guid()); On 2013/12/18 21:26:49, rlarocque wrote: > On 2013/12/18 20:45:30, albertb wrote: > > The client should be setting originator_cache_guid, we never modify that field > > on the server. > > Are you sure? I'm pretty sure we're not setting it on the client. > > "git grep set_originator_cache_guid" turns up nothing but unit tests on the > client. Ah, I was slightly confused. Looking at the code: - Both originator_cache_guid are only relevant for CLIENT types (eg. bookmarks) - On commit, we grab the client_id parameter from the URL and use it to derive IdString - On GetUpdates, we read client_id from the IdString and set originator_cache_guid on the SyncEntity The same applies to originator_client_item_id. What this means is that for bookmarks, on the first commit (ie. version==0), FakeServer should: - Set originator_cache_guid to the client_id parameter from the request URL - Set originator_client_item_id to the IdString sent up by the client. For all other commits, FakeServer shouldn't touch that field.
Here are a few preliminary comments on gyp targets and nomenclature. https://codereview.chromium.org/115243007/diff/40001/chrome/browser/sync/test... File chrome/browser/sync/test/integration/prototype_fake_server_test.cc (right): https://codereview.chromium.org/115243007/diff/40001/chrome/browser/sync/test... chrome/browser/sync/test/integration/prototype_fake_server_test.cc:23: ASSERT_TRUE(SetupSync()) << "SetupSync() failed."; Considering that this test is primarily meant to test the fake server as opposed to the sync engine, perhaps you could verify that the server is running before calling SetupSync? And after sync is set up, I wonder if it makes sense to verify that the server contains the same data as the client. Feel free to do this now or in a future CL. https://codereview.chromium.org/115243007/diff/40001/chrome/browser/sync/test... File chrome/browser/sync/test/integration/sync_test.h (right): https://codereview.chromium.org/115243007/diff/40001/chrome/browser/sync/test... chrome/browser/sync/test/integration/sync_test.h:69: FAKE_SERVER, // The fake, in-process server (FakeServer) that On 2013/12/18 20:22:01, rlarocque wrote: > How about IN_PROCESS_SERVER? Maybe someone else on the review has a better > idea. I like the name IN_PROCESS_SERVER, but there's already an in-process server in net/test/embedded_test_server/embedded_test_server.h. It's started with every browser test, but the sync integration tests don't use it. Maybe call this IN_PROCESS_SYNC_SERVER? Also, it might make sense to rename the files fake_server.* based on what name you pick here. https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/public... File sync/internal_api/public/test/fake_server.h (right): https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/public... sync/internal_api/public/test/fake_server.h:18: // An in-process implementation of the Sync server used for testing. Desktop in-process browser tests already have the notion of an embedded test server. It may be worth elaborating on why we're adding yet another test server class instead of reusing the infrastructure in net/test/embedded_test_server/embedded_test_server.h. https://codereview.chromium.org/115243007/diff/40001/sync/sync_tests.gypi File sync/sync_tests.gypi (right): https://codereview.chromium.org/115243007/diff/40001/sync/sync_tests.gypi#new... sync/sync_tests.gypi:154: 'internal_api/public/test/fake_server.h', These should be a part of test_support_sync_testserver and not test_support_sync_internal_api. Also, it probably makes sense to move some / all of the files that implement the fake server into a separate directory. We currently have src/sync/tools/testserver and src/sync/test, but I'm not against adding a new directory for the C++ fake server.
I didn't have enough time to address all comments, so I created an artificial stopping point and am publishing those changes here. We can start this review back up again in the new year. https://codereview.chromium.org/115243007/diff/40001/chrome/browser/sync/test... File chrome/browser/sync/test/integration/prototype_fake_server_test.cc (right): https://codereview.chromium.org/115243007/diff/40001/chrome/browser/sync/test... chrome/browser/sync/test/integration/prototype_fake_server_test.cc:19: // TODO(pvalenzuela): Remove this test when sync_integration_tests is On 2013/12/18 20:22:01, rlarocque wrote: > You should reference the crbug for the fake server development in this comment. Done. https://codereview.chromium.org/115243007/diff/40001/chrome/browser/sync/test... chrome/browser/sync/test/integration/prototype_fake_server_test.cc:23: ASSERT_TRUE(SetupSync()) << "SetupSync() failed."; On 2013/12/18 22:37:12, Raghu Simha wrote: > Considering that this test is primarily meant to test the fake server as opposed > to the sync engine, perhaps you could verify that the server is running before > calling SetupSync? And after sync is set up, I wonder if it makes sense to > verify that the server contains the same data as the client. Feel free to do > this now or in a future CL. I like this idea, but would prefer to do it in a future CL. https://codereview.chromium.org/115243007/diff/40001/chrome/browser/sync/test... File chrome/browser/sync/test/integration/sync_test.cc (right): https://codereview.chromium.org/115243007/diff/40001/chrome/browser/sync/test... chrome/browser/sync/test/integration/sync_test.cc:880: server_type_ = FAKE_SERVER; On 2013/12/18 20:22:01, rlarocque wrote: > Can you somehow DCHECK that SetUp() hasn't been called yet? Maybe > DCHECK_EQ(SERVER_TYPE_UNDECIDED, server_type_) would do the trick? Great idea. Thanks. https://codereview.chromium.org/115243007/diff/40001/chrome/browser/sync/test... File chrome/browser/sync/test/integration/sync_test.h (right): https://codereview.chromium.org/115243007/diff/40001/chrome/browser/sync/test... chrome/browser/sync/test/integration/sync_test.h:69: FAKE_SERVER, // The fake, in-process server (FakeServer) that On 2013/12/18 20:22:01, rlarocque wrote: > I'm not sure if FAKE_SERVER is the best name for this. Wouldn't that be an > equally appropriate name for the python server? > > How about IN_PROCESS_SERVER? Maybe someone else on the review has a better > idea. I changed this to IN_PROCESS_FAKE_SERVER so that it fully describes the setup: the fake server running in-process. https://codereview.chromium.org/115243007/diff/40001/chrome/browser/sync/test... chrome/browser/sync/test/integration/sync_test.h:69: FAKE_SERVER, // The fake, in-process server (FakeServer) that On 2013/12/18 22:37:12, Raghu Simha wrote: > On 2013/12/18 20:22:01, rlarocque wrote: > > How about IN_PROCESS_SERVER? Maybe someone else on the review has a better > > idea. > > I like the name IN_PROCESS_SERVER, but there's already an in-process server in > net/test/embedded_test_server/embedded_test_server.h. It's started with every > browser test, but the sync integration tests don't use it. Maybe call this > IN_PROCESS_SYNC_SERVER? > > Also, it might make sense to rename the files fake_server.* based on what name > you pick here. I think the IN_PROCESS_FAKE_SERVER naming makes it clear what's happening here. I'd prefer to not use IN_PROCESS_SYNC_SERVER because it sounds like it's a real Sync server being used. As for renaming the FakeServer-related classes, I'd like to keep those as is (it's the fake server and how it's used doesn't really matter). https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/public... File sync/internal_api/public/test/fake_server.h (right): https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/public... sync/internal_api/public/test/fake_server.h:18: // An in-process implementation of the Sync server used for testing. On 2013/12/18 22:37:12, Raghu Simha wrote: > Desktop in-process browser tests already have the notion of an embedded test > server. It may be worth elaborating on why we're adding yet another test server > class instead of reusing the infrastructure in > net/test/embedded_test_server/embedded_test_server.h. I modified this comment to remove the mention of running it in-process; FakeServer is agnostic to how its being used (e.g., in-process as in this CL, EmbeddedTestServer, separate binary). I've added a TODO in sync_test.cc to use EmbeddedTestServer for desktop tests. After our discussions, I think that is the right direction going forward. We need this new in-process method for iOS; where do you think I should mention that? https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/public... sync/internal_api/public/test/fake_server.h:19: class FakeServer { On 2013/12/18 20:22:01, rlarocque wrote: > General comment: > > I think you'll find it easier in the long run to keep up with server and client > features if you split up functionality for different types. Certain types, like > experiments or push messaging, are not like the others. If you continue down > the current path, you might one day find your code littered with if (type == > PUSH_MESSAGING) or (type == BOOKMARKS) conditions all over the place. > > If you implement one class (or at least one instance of some common class) per > type, you might have an easier time handling each type's special features. > > You don't need to act on this comment in this CL, or at all. However, if you > agree that splitting up functionality per type is more future proof, you'll > probably want to start with that design from the start rather than trying to > refactor it in later. This makes sense. For this CL, I'll keep this as is and revisit the idea in the next CL. https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/public... sync/internal_api/public/test/fake_server.h:30: std::string HandleCommand(std::string request, int* error_code, On 2013/12/18 20:22:01, rlarocque wrote: > nit: one parameter per line. > > http://www.chromium.org/developers/coding-style#Code_Formatting > > Also, it seems a little odd to me that this combines both out parameters and a > return value in this way. Normally the pattern would be something like > > int error = DoFailableAction(&result); > > where there is some sort of guarantee that result is not modified when the > return value indicates an error. Done. https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/public... sync/internal_api/public/test/fake_server.h:37: class UpdateSieve { On 2013/12/18 20:22:01, rlarocque wrote: > I see few advantages to using inner-classes in C++. > > Since you declared it private, I'm guessing your intention is to hide it. If > that's the case, you're probably better off declaring it in an anonymous > namespace in the .cpp file. Done. https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/public... sync/internal_api/public/test/fake_server.h:62: typedef std::map<int, int64> TypeIdToVersionMap; On 2013/12/18 20:22:01, rlarocque wrote: > Since we're in C++ land and you have the classes available, you'd might as well > make this a map from ModelType to int64. Done. https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/public... sync/internal_api/public/test/fake_server.h:67: // Maps data type IDs to the latest version seen for that version. On 2013/12/18 20:22:01, rlarocque wrote: > "version seen for that version" -> typo? Done. https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/public... sync/internal_api/public/test/fake_server.h:68: TypeIdToVersionMap state_; On 2013/12/18 20:22:01, rlarocque wrote: > state_ is not a very informative name. How about: > - type_progress_ > - type_min_versions_ > > other suggestions welcome. how about type_latest_version_?
Here are the rest of the comment responses. https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... File sync/internal_api/test/fake_server.cc (right): https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... sync/internal_api/test/fake_server.cc:20: static const std::string kRootParentTag = "0"; On 2013/12/18 20:22:01, rlarocque wrote: > I'm not 100% sure about this, but I think this line creates a static > initializer. > > Declaring this as a char[] would be a bit uglier, but would almost certainly > prevent the creation of a static initializer. > > On the other hand, this is test code that only gets compiled into a test binary, > so I don't know if we care either way. I'm fine with leaving it here if there are no awful consequences. Should this be in the syncer namespace instead? https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... sync/internal_api/test/fake_server.cc:46: state_[ModelTypeFromInt(TOP_LEVEL_FOLDER)] = min_version_; On 2013/12/18 20:22:01, rlarocque wrote: > This looks sketchy to me. > > TOP_LEVEL_FOLDER is enough of an exception that it might be better keep it out > of the map and (if necessary) store its version in a separate member variable. > > You might need more if statements in ClientWantsItem(), but you would need less > special case code here and in UpdateProgressMarkers. This seems less > error-prone. > > FWIW, the client is slightly inconsistent about this, but in general it tries to > treat TOP_LEVEL_FOLDER items as having the same type as their children. So the > node with SERVER_UNIQUE_TAG of google_chrome_preferences would often be observed > as having type PREFERENCES. I've removed handling of TOP_LEVEL_FOLDER since it never goes over the wire and, AFAIK, will never show up here. https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... sync/internal_api/test/fake_server.cc:85: if (state_[it->first] == 0) On 2013/12/18 20:22:01, rlarocque wrote: > Could TOP_LEVEL_FOLDER wind up in this list of types? Would that make sense? Not anymore (see comment above about removing it entirely). https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... sync/internal_api/test/fake_server.cc:114: spec.parent_tag = kRootParentTag; On 2013/12/18 20:22:01, rlarocque wrote: > It seems you're using tags as IDs? > > The root node's ID should be "r". Is it only "r" on the client side? It appears that "0" is used for the server: https://cs.corp.google.com/#piper///depot/google3/java/com/google/personaliza... https://code.google.com/p/chromium/codesearch#chromium/src/sync/syncable/sync... The current Python test server also uses "0": https://code.google.com/p/chromium/codesearch#chromium/src/sync/tools/testser... https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... sync/internal_api/test/fake_server.cc:135: On 2013/12/18 20:22:01, rlarocque wrote: > In certain situations, you should create the mobile bookmarks folder, too. If > you see your tests crashing in strange ways on mobile clients, this may be the > reason why. If they don't crash, feel free to ignore this comment and implement > that feature later. Thanks; I added a TODO for this. https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... sync/internal_api/test/fake_server.cc:141: for (size_t i = 0; i < permanent_item_specs_.size(); i++) { On 2013/12/18 20:22:01, rlarocque wrote: > Why is the creation of permanent items a two-step process? > > Rather than creating a bunch of PermanentItemSpecs then iterating over them to > transform them into SyncEntities, couldn't you just create them as SyncEntities > from the start? This was a result of following the Python server's implementation too closely. Your idea is great and it's cleaned up this code immensely. As a result, I've also removed the Init() method. https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... sync/internal_api/test/fake_server.cc:166: for (size_t i = 0; i < permanent_item_specs_.size(); i++) { On 2013/12/18 20:22:01, rlarocque wrote: > nit: You should prefer to use STL iterators rather than indexes where possible. Done. https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... sync/internal_api/test/fake_server.cc:168: entity.set_parent_id_string(permanent_item_specs_[i].tag); On 2013/12/18 20:22:01, rlarocque wrote: > This looks like it's redundant. Can't you just call > entity.set_parent_id_string(spec.parent_tag) and skip this iteration? Yes. I've made this change in the new version. https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... sync/internal_api/test/fake_server.cc:170: entity.set_position_in_parent(1337); On 2013/12/18 20:22:01, rlarocque wrote: > I think this should be unset on all items that are neither bookmarks nor > bookmark folders. Done. https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... sync/internal_api/test/fake_server.cc:191: entity.set_originator_cache_guid(original_entity.originator_cache_guid()); On 2013/12/18 21:47:30, albertb wrote: > On 2013/12/18 21:26:49, rlarocque wrote: > > On 2013/12/18 20:45:30, albertb wrote: > > > The client should be setting originator_cache_guid, we never modify that > field > > > on the server. > > > > Are you sure? I'm pretty sure we're not setting it on the client. > > > > "git grep set_originator_cache_guid" turns up nothing but unit tests on the > > client. > > Ah, I was slightly confused. Looking at the code: > > - Both originator_cache_guid are only relevant for CLIENT types (eg. bookmarks) > - On commit, we grab the client_id parameter from the URL and use it to derive > IdString > - On GetUpdates, we read client_id from the IdString and set > originator_cache_guid on the SyncEntity > > The same applies to originator_client_item_id. > > What this means is that for bookmarks, on the first commit (ie. version==0), > FakeServer should: > - Set originator_cache_guid to the client_id parameter from the request URL > - Set originator_client_item_id to the IdString sent up by the client. > > For all other commits, FakeServer shouldn't touch that field. Thanks for the explanation. I will follow this commit logic when I implement the commit command. I based this code on the Python test server. It appears this is done so that the server always maintains both fields. Is this right, wrong, or simply unnecessary? https://code.google.com/p/chromium/codesearch#chromium/src/sync/tools/testser... https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... sync/internal_api/test/fake_server.cc:233: get_updates_response->set_changes_remaining(0); On 2013/12/18 20:45:30, albertb wrote: > Will all SyncEntities always fit in a single response? Shouldn't FakeServer > respect GetUpdatesMessage.batch_size? I've added a TODO for this. https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... sync/internal_api/test/fake_server.cc:264: if (sending_nigori || message.get_updates().need_encryption_key()) { On 2013/12/18 20:45:30, albertb wrote: > Note that sending_nigori does not necessarily imply that you should add > encryption_keys to the response. For custom passphrase users, we never send back > encryption keys. Thanks, I added some logic here for this. https://codereview.chromium.org/115243007/diff/40001/sync/sync_tests.gypi File sync/sync_tests.gypi (right): https://codereview.chromium.org/115243007/diff/40001/sync/sync_tests.gypi#new... sync/sync_tests.gypi:154: 'internal_api/public/test/fake_server.h', On 2013/12/18 22:37:12, Raghu Simha wrote: > These should be a part of test_support_sync_testserver and not > test_support_sync_internal_api. Also, it probably makes sense to move some / all > of the files that implement the fake server into a separate directory. We > currently have src/sync/tools/testserver and src/sync/test, but I'm not against > adding a new directory for the C++ fake server. I'll make these changes at the end of the review for the sake of preserving existing review comments.
Not many comments left. https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... File sync/internal_api/test/fake_server.cc (right): https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... sync/internal_api/test/fake_server.cc:20: static const std::string kRootParentTag = "0"; On 2014/01/03 23:25:31, pvalenzuela wrote: > On 2013/12/18 20:22:01, rlarocque wrote: > > I'm not 100% sure about this, but I think this line creates a static > > initializer. > > > > Declaring this as a char[] would be a bit uglier, but would almost certainly > > prevent the creation of a static initializer. > > > > On the other hand, this is test code that only gets compiled into a test > binary, > > so I don't know if we care either way. > > I'm fine with leaving it here if there are no awful consequences. Should this be > in the syncer namespace instead? The namespace doesn't matter too much. Because it's declared file static it will be hidden from all contexts outside of this file anyway. https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... sync/internal_api/test/fake_server.cc:114: spec.parent_tag = kRootParentTag; On 2014/01/03 23:25:31, pvalenzuela wrote: > On 2013/12/18 20:22:01, rlarocque wrote: > > It seems you're using tags as IDs? > > > > The root node's ID should be "r". > > Is it only "r" on the client side? > > It appears that "0" is used for the server: > https://cs.corp.google.com/#piper///depot/google3/java/com/google/personaliza... > > https://code.google.com/p/chromium/codesearch#chromium/src/sync/syncable/sync... > > The current Python test server also uses "0": > https://code.google.com/p/chromium/codesearch#chromium/src/sync/tools/testser... > Looks like you're right. The server refers to is as "0". https://codereview.chromium.org/115243007/diff/110001/sync/internal_api/test/... File sync/internal_api/test/fake_server.cc (right): https://codereview.chromium.org/115243007/diff/110001/sync/internal_api/test/... sync/internal_api/test/fake_server.cc:26: // A filter used during GetUpdates calls to determine what information to You don't need to indent for the namespace. https://codereview.chromium.org/115243007/diff/110001/sync/internal_api/test/... sync/internal_api/test/fake_server.cc:44: version = 0; This is a good example of why doing work in a constructor is a bad idea. If the progress marker fails to parse, you really shouldn't treat it as having version zero.* You should report some sort of error. But this code is in a constructor and we've banned exceptions, so there's no way to fail. Here's an alternative. Make a static factory method that parses the incoming GetUpdatesMessage and returns an UpdateSieve*. You can return a 400 response or DCHECK if parsing fails. As a bonus, you can have a simple, private constructor that contains only member initializers. This will allow you to make some or all of these class members const. * We'll need to handle the first request specially, of course. I think the client's first request will have an empty token. We can special case that condition, and still handle any parse failures separately. https://codereview.chromium.org/115243007/diff/110001/sync/internal_api/test/... sync/internal_api/test/fake_server.cc:75: int64 version = new_version > state_[model_type] ? nit: use std::max instead? Also, what does this do? Why would we want to return state_[model_type] as the version if the highest version entry we sent back is less than it? https://codereview.chromium.org/115243007/diff/110001/sync/internal_api/test/... sync/internal_api/test/fake_server.cc:113: ModelTypeToVersionMap state_; I think state_ is not a very good name. How about request_from_version_? https://codereview.chromium.org/115243007/diff/110001/sync/internal_api/test/... sync/internal_api/test/fake_server.cc:258: for (std::vector<sync_pb::SyncEntity>::iterator it = I don't think you gain much by iterating twice. Couldn't you iterate over entities_ rather than filtered_entities here?
https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... File sync/internal_api/test/fake_server.cc (right): https://codereview.chromium.org/115243007/diff/40001/sync/internal_api/test/f... sync/internal_api/test/fake_server.cc:191: entity.set_originator_cache_guid(original_entity.originator_cache_guid()); On 2014/01/03 23:25:31, pvalenzuela wrote: > On 2013/12/18 21:47:30, albertb wrote: > > On 2013/12/18 21:26:49, rlarocque wrote: > > > On 2013/12/18 20:45:30, albertb wrote: > > > > The client should be setting originator_cache_guid, we never modify that > > field > > > > on the server. > > > > > > Are you sure? I'm pretty sure we're not setting it on the client. > > > > > > "git grep set_originator_cache_guid" turns up nothing but unit tests on the > > > client. > > > > Ah, I was slightly confused. Looking at the code: > > > > - Both originator_cache_guid are only relevant for CLIENT types (eg. > bookmarks) > > - On commit, we grab the client_id parameter from the URL and use it to derive > > IdString > > - On GetUpdates, we read client_id from the IdString and set > > originator_cache_guid on the SyncEntity > > > > The same applies to originator_client_item_id. > > > > What this means is that for bookmarks, on the first commit (ie. version==0), > > FakeServer should: > > - Set originator_cache_guid to the client_id parameter from the request URL > > - Set originator_client_item_id to the IdString sent up by the client. > > > > For all other commits, FakeServer shouldn't touch that field. > > Thanks for the explanation. > > I will follow this commit logic when I implement the commit command. > > I based this code on the Python test server. It appears this is done so that the > server always maintains both fields. Is this right, wrong, or simply > unnecessary? > > https://code.google.com/p/chromium/codesearch#chromium/src/sync/tools/testser... That does look correct. The server should preserve the initial value (ie. version==0) of those fields, regardless of whether further commits modify or omit them.
https://codereview.chromium.org/115243007/diff/110001/sync/internal_api/test/... File sync/internal_api/test/fake_server.cc (right): https://codereview.chromium.org/115243007/diff/110001/sync/internal_api/test/... sync/internal_api/test/fake_server.cc:26: // A filter used during GetUpdates calls to determine what information to On 2014/01/04 00:06:24, rlarocque wrote: > You don't need to indent for the namespace. oops, fixed https://codereview.chromium.org/115243007/diff/110001/sync/internal_api/test/... sync/internal_api/test/fake_server.cc:44: version = 0; On 2014/01/04 00:06:24, rlarocque wrote: > This is a good example of why doing work in a constructor is a bad idea. If the > progress marker fails to parse, you really shouldn't treat it as having version > zero.* You should report some sort of error. But this code is in a constructor > and we've banned exceptions, so there's no way to fail. > > Here's an alternative. Make a static factory method that parses the incoming > GetUpdatesMessage and returns an UpdateSieve*. You can return a 400 response or > DCHECK if parsing fails. > > As a bonus, you can have a simple, private constructor that contains only member > initializers. This will allow you to make some or all of these class members > const. > > * We'll need to handle the first request specially, of course. I think the > client's first request will have an empty token. We can special case that > condition, and still handle any parse failures separately. I like this idea. I'll make the change, but how can we determine if a given request is the first request? https://codereview.chromium.org/115243007/diff/110001/sync/internal_api/test/... sync/internal_api/test/fake_server.cc:75: int64 version = new_version > state_[model_type] ? On 2014/01/04 00:06:24, rlarocque wrote: > nit: use std::max instead? > > Also, what does this do? Why would we want to return state_[model_type] as the > version if the highest version entry we sent back is less than it? Changed to use std::max. As for the purpose, I believe this is only useful when new_version is 0 (this is the case where the server has no entities to sent back in the GetUpdatesResponse). In this case, we shouldn't update the progress marker version since nothing changed. Does that make sense? Most of what I'm going from here is the Python test server. https://codereview.chromium.org/115243007/diff/110001/sync/internal_api/test/... sync/internal_api/test/fake_server.cc:113: ModelTypeToVersionMap state_; On 2014/01/04 00:06:24, rlarocque wrote: > I think state_ is not a very good name. How about request_from_version_? Done. https://codereview.chromium.org/115243007/diff/110001/sync/internal_api/test/... sync/internal_api/test/fake_server.cc:258: for (std::vector<sync_pb::SyncEntity>::iterator it = On 2014/01/04 00:06:24, rlarocque wrote: > I don't think you gain much by iterating twice. Couldn't you iterate over > entities_ rather than filtered_entities here? Ah, much nicer. Thanks.
friendly ping @rlarocque @albertb: Any other comments? After any further comments are resolved, I'll move files as suggested by @rsimha earlier in the review thread.
https://codereview.chromium.org/115243007/diff/110001/sync/internal_api/test/... File sync/internal_api/test/fake_server.cc (right): https://codereview.chromium.org/115243007/diff/110001/sync/internal_api/test/... sync/internal_api/test/fake_server.cc:44: version = 0; On 2014/01/06 19:24:32, pvalenzuela wrote: > On 2014/01/04 00:06:24, rlarocque wrote: > > This is a good example of why doing work in a constructor is a bad idea. If > the > > progress marker fails to parse, you really shouldn't treat it as having > version > > zero.* You should report some sort of error. But this code is in a > constructor > > and we've banned exceptions, so there's no way to fail. > > > > Here's an alternative. Make a static factory method that parses the incoming > > GetUpdatesMessage and returns an UpdateSieve*. You can return a 400 response > or > > DCHECK if parsing fails. > > > > As a bonus, you can have a simple, private constructor that contains only > member > > initializers. This will allow you to make some or all of these class members > > const. > > > > * We'll need to handle the first request specially, of course. I think the > > client's first request will have an empty token. We can special case that > > condition, and still handle any parse failures separately. > > I like this idea. I'll make the change, but how can we determine if a given > request is the first request? The first request will have an empty progress token. The code should be made to fail when parsing the token fails unless the token is empty. In that case parsing would fail if attempted, but the code will bypass that logic and simply assume that the request is the first one for this type.
I don't think I have any big comments outstanding. It's fine with me if you want to move the files now.
I've added the UpdateSieve factory method and moved the files to sync/test/fake_server (separate patchsets). https://codereview.chromium.org/115243007/diff/110001/sync/internal_api/test/... File sync/internal_api/test/fake_server.cc (right): https://codereview.chromium.org/115243007/diff/110001/sync/internal_api/test/... sync/internal_api/test/fake_server.cc:44: version = 0; On 2014/01/10 20:09:41, rlarocque wrote: > On 2014/01/06 19:24:32, pvalenzuela wrote: > > On 2014/01/04 00:06:24, rlarocque wrote: > > > This is a good example of why doing work in a constructor is a bad idea. If > > the > > > progress marker fails to parse, you really shouldn't treat it as having > > version > > > zero.* You should report some sort of error. But this code is in a > > constructor > > > and we've banned exceptions, so there's no way to fail. > > > > > > Here's an alternative. Make a static factory method that parses the > incoming > > > GetUpdatesMessage and returns an UpdateSieve*. You can return a 400 > response > > or > > > DCHECK if parsing fails. > > > > > > As a bonus, you can have a simple, private constructor that contains only > > member > > > initializers. This will allow you to make some or all of these class > members > > > const. > > > > > > * We'll need to handle the first request specially, of course. I think the > > > client's first request will have an empty token. We can special case that > > > condition, and still handle any parse failures separately. > > > > I like this idea. I'll make the change, but how can we determine if a given > > request is the first request? > > The first request will have an empty progress token. The code should be made to > fail when parsing the token fails unless the token is empty. In that case > parsing would fail if attempted, but the code will bypass that logic and simply > assume that the request is the first one for this type. I've added the factory method and considered the first request case. PTAL.
I think that's the last of my comments. LGTM. https://codereview.chromium.org/115243007/diff/470001/sync/test/fake_server/f... File sync/test/fake_server/fake_server.cc (right): https://codereview.chromium.org/115243007/diff/470001/sync/test/fake_server/f... sync/test/fake_server/fake_server.cc:32: static scoped_ptr<UpdateSieve> Create( nit that I should have caught earlier: You shouldn't declare these methods inline. Even in a .cc file it's still better to declare the functions outside the class.
Raghu, any additional comments? https://codereview.chromium.org/115243007/diff/470001/sync/test/fake_server/f... File sync/test/fake_server/fake_server.cc (right): https://codereview.chromium.org/115243007/diff/470001/sync/test/fake_server/f... sync/test/fake_server/fake_server.cc:32: static scoped_ptr<UpdateSieve> Create( On 2014/01/14 01:53:38, rlarocque wrote: > nit that I should have caught earlier: > > You shouldn't declare these methods inline. Even in a .cc file it's still > better to declare the functions outside the class. Thanks. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pvalenzuela@chromium.org/115243007/660001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pvalenzuela@chromium.org/115243007/660001
Retried try job too often on android_clang_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pvalenzuela@chromium.org/115243007/114...
Retried try job too often on win_rel for step(s) base_unittests, base_unittests_swarm http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pvalenzuela@chromium.org/115243007/114...
Message was sent while issue was closed.
Change committed as 245326 |
