Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2372903002:
Helium: Adding vector clock (Closed)
|
Created:
4 years, 2 months ago by scf Modified:
4 years, 2 months ago CC:
cbentzel+watch_chromium.org, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, gcasto+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, perumaal+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org, shaktisahu+watch-blimp_chromium.org, khushalsagar+watch-blimp_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHelium: Initial Implementation of vector clock
Adding initial interface with methods to compare, merge two vectors and increment a local version of a vector clock.
BUG=650461
Committed: https://crrev.com/4debff3c473712310b30ac84924f322ed2a2d7ed
Cr-Commit-Position: refs/heads/master@{#421573}
Patch Set 1 #Patch Set 2 : Helium proto file and Vector Clock interface #Patch Set 3 : Helium: Adding vector clock #Patch Set 4 : allowing merge with conflicted vectors #
Total comments: 28
Patch Set 5 : Addressing review comments #Patch Set 6 : Missed one feedback (s/r/Conflicts/Conflict) #
Total comments: 29
Patch Set 7 : addressing feedback #Patch Set 8 : Missed to update one revision #
Total comments: 10
Patch Set 9 : addressing changes #Patch Set 10 : rebase #Patch Set 11 : Fixing CQ issue regarding net export #
Messages
Total messages: 31 (15 generated)
Description was changed from ========== initial proto BUG= ========== to ========== Initial implementation of vector clock BUG= ==========
scf@chromium.org changed reviewers: + kmarshall@chromium.org, steimel@chromium.org
https://codereview.chromium.org/2372903002/diff/60001/blimp/net/BUILD.gn File blimp/net/BUILD.gn (right): https://codereview.chromium.org/2372903002/diff/60001/blimp/net/BUILD.gn#newc... blimp/net/BUILD.gn:55: "helium/vector_clock.cc", Recommend putting this under a source_set("helium") https://codereview.chromium.org/2372903002/diff/60001/blimp/net/helium/vector... File blimp/net/helium/vector_clock.cc (right): https://codereview.chromium.org/2372903002/diff/60001/blimp/net/helium/vector... blimp/net/helium/vector_clock.cc:6: #include <algorithm> Add newline before and after <algorithm> https://codereview.chromium.org/2372903002/diff/60001/blimp/net/helium/vector... blimp/net/helium/vector_clock.cc:14: VectorClock::Comparison VectorClock::CompareTo(const VectorClock& other) const { Add DCHECK(local_revision_ >= other.local_revision_) at the top. https://codereview.chromium.org/2372903002/diff/60001/blimp/net/helium/vector... blimp/net/helium/vector_clock.cc:31: // its smaller < Can you rework comment to be a well formed sentence? Also: Maybe consider appending the comment to "else", so you'd have: } else { // (local_revision_ == other.local_revision_) https://codereview.chromium.org/2372903002/diff/60001/blimp/net/helium/vector... blimp/net/helium/vector_clock.cc:40: VectorClock VectorClock::MergeWith(const VectorClock& other) { add newline above https://codereview.chromium.org/2372903002/diff/60001/blimp/net/helium/vector... blimp/net/helium/vector_clock.cc:41: VectorClock result(std::max(local_revision_, other.local_revision_), Can you use the getter() to access |other|'s members, here and elsewhere in the file? https://codereview.chromium.org/2372903002/diff/60001/blimp/net/helium/vector... blimp/net/helium/vector_clock.cc:45: void VectorClock::IncrementLocal() { add newline above https://codereview.chromium.org/2372903002/diff/60001/blimp/net/helium/vector... File blimp/net/helium/vector_clock.h (right): https://codereview.chromium.org/2372903002/diff/60001/blimp/net/helium/vector... blimp/net/helium/vector_clock.h:8: #include <cinttypes> <stdint.h> is used more frequently https://codereview.chromium.org/2372903002/diff/60001/blimp/net/helium/vector... blimp/net/helium/vector_clock.h:14: class BLIMP_NET_EXPORT VectorClock { Add comment to class describing what vector clocks do, how this class implements them, where it might be used, and links to reference materials. A lot of people are unfamiliar with vector clocks so this file will be the first place they'll look for more information. https://codereview.chromium.org/2372903002/diff/60001/blimp/net/helium/vector... blimp/net/helium/vector_clock.h:16: VectorClock(int32_t local_revision, int32_t remote_revision); Does it make sense to have a default -public constructor for this class, or should we declare it under private? https://codereview.chromium.org/2372903002/diff/60001/blimp/net/helium/vector... blimp/net/helium/vector_clock.h:18: enum class Comparison { LessThan, EqualTo, GreaterThan, Conflicts }; Potentially misguided language nit: What about "Conflict", singular? Otherwise we would need "ConflictsWith" if we maintained the convention of the previous three items. https://codereview.chromium.org/2372903002/diff/60001/blimp/net/helium/vector... blimp/net/helium/vector_clock.h:18: enum class Comparison { LessThan, EqualTo, GreaterThan, Conflicts }; Enums go at the top of public: https://codereview.chromium.org/2372903002/diff/60001/blimp/net/helium/vector... blimp/net/helium/vector_clock.h:20: Comparison CompareTo(const VectorClock& other) const; Add comments for all public members except maybe the revision getters (those are pretty obvious) https://codereview.chromium.org/2372903002/diff/60001/blimp/net/helium/vector... File blimp/net/helium/vector_clock_unittest.cc (right): https://codereview.chromium.org/2372903002/diff/60001/blimp/net/helium/vector... blimp/net/helium/vector_clock_unittest.cc:29: INSTANTIATE_TEST_CASE_P( Wow, this is pretty cool. I was unaware of this style of unit test! https://codereview.chromium.org/2372903002/diff/60001/blimp/net/helium/vector... blimp/net/helium/vector_clock_unittest.cc:99: VectorClock v1(1, 2); We could cut down on the number of test cases by making a helper function that verifies the associativity of merges. VerifyMerge(1, 2, 1, 4) ==> merge((1, 2), (1, 4)); merge((1, 4), (1,2))
https://codereview.chromium.org/2372903002/diff/60001/blimp/net/BUILD.gn File blimp/net/BUILD.gn (right): https://codereview.chromium.org/2372903002/diff/60001/blimp/net/BUILD.gn#newc... blimp/net/BUILD.gn:55: "helium/vector_clock.cc", On 2016/09/27 17:31:09, Kevin M wrote: > Recommend putting this under a source_set("helium") Done. https://codereview.chromium.org/2372903002/diff/60001/blimp/net/helium/vector... File blimp/net/helium/vector_clock.cc (right): https://codereview.chromium.org/2372903002/diff/60001/blimp/net/helium/vector... blimp/net/helium/vector_clock.cc:6: #include <algorithm> On 2016/09/27 17:31:09, Kevin M wrote: > Add newline before and after <algorithm> Done. https://codereview.chromium.org/2372903002/diff/60001/blimp/net/helium/vector... blimp/net/helium/vector_clock.cc:14: VectorClock::Comparison VectorClock::CompareTo(const VectorClock& other) const { On 2016/09/27 17:31:09, Kevin M wrote: > Add DCHECK(local_revision_ >= other.local_revision_) at the top. Done. https://codereview.chromium.org/2372903002/diff/60001/blimp/net/helium/vector... blimp/net/helium/vector_clock.cc:31: // its smaller < On 2016/09/27 17:31:09, Kevin M wrote: > Can you rework comment to be a well formed sentence? > > Also: > > Maybe consider appending the comment to "else", so you'd have: > > } else { // (local_revision_ == other.local_revision_) Done. https://codereview.chromium.org/2372903002/diff/60001/blimp/net/helium/vector... blimp/net/helium/vector_clock.cc:40: VectorClock VectorClock::MergeWith(const VectorClock& other) { On 2016/09/27 17:31:09, Kevin M wrote: > add newline above Done. https://codereview.chromium.org/2372903002/diff/60001/blimp/net/helium/vector... blimp/net/helium/vector_clock.cc:41: VectorClock result(std::max(local_revision_, other.local_revision_), On 2016/09/27 17:31:09, Kevin M wrote: > Can you use the getter() to access |other|'s members, here and elsewhere in the > file? Done. https://codereview.chromium.org/2372903002/diff/60001/blimp/net/helium/vector... blimp/net/helium/vector_clock.cc:45: void VectorClock::IncrementLocal() { On 2016/09/27 17:31:09, Kevin M wrote: > add newline above Done. https://codereview.chromium.org/2372903002/diff/60001/blimp/net/helium/vector... File blimp/net/helium/vector_clock.h (right): https://codereview.chromium.org/2372903002/diff/60001/blimp/net/helium/vector... blimp/net/helium/vector_clock.h:8: #include <cinttypes> On 2016/09/27 17:31:10, Kevin M wrote: > <stdint.h> is used more frequently Done. https://codereview.chromium.org/2372903002/diff/60001/blimp/net/helium/vector... blimp/net/helium/vector_clock.h:14: class BLIMP_NET_EXPORT VectorClock { On 2016/09/27 17:31:10, Kevin M wrote: > Add comment to class describing what vector clocks do, how this class implements > them, where it might be used, and links to reference materials. A lot of people > are unfamiliar with vector clocks so this file will be the first place they'll > look for more information. Done. https://codereview.chromium.org/2372903002/diff/60001/blimp/net/helium/vector... blimp/net/helium/vector_clock.h:16: VectorClock(int32_t local_revision, int32_t remote_revision); On 2016/09/27 17:31:10, Kevin M wrote: > Does it make sense to have a default -public constructor for this class, or > should we declare it under private? Agree, default public makes sense. https://codereview.chromium.org/2372903002/diff/60001/blimp/net/helium/vector... blimp/net/helium/vector_clock.h:18: enum class Comparison { LessThan, EqualTo, GreaterThan, Conflicts }; On 2016/09/27 17:31:10, Kevin M wrote: > Enums go at the top of public: Done. https://codereview.chromium.org/2372903002/diff/60001/blimp/net/helium/vector... blimp/net/helium/vector_clock.h:18: enum class Comparison { LessThan, EqualTo, GreaterThan, Conflicts }; On 2016/09/27 17:31:10, Kevin M wrote: > Enums go at the top of public: Done. https://codereview.chromium.org/2372903002/diff/60001/blimp/net/helium/vector... blimp/net/helium/vector_clock.h:20: Comparison CompareTo(const VectorClock& other) const; On 2016/09/27 17:31:10, Kevin M wrote: > Add comments for all public members except maybe the revision getters (those are > pretty obvious) Done.
Just some comment and formatting style nits. https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock.cc (right): https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.cc:19: DCHECK(local_revision() >= other.local_revision()); add blank line below dchecks https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.cc:39: DCHECK(false) << "Local revision should always be greater or equal."; * Already checked in the top of CompareTo. * No need to add explanation; that DCHECK body is self explanatory. * DCHECK(false) should be DLOG(FATAL) https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.cc:53: set_local_revision(local_revision() + 1); Accesses to members of |this| needn't be mediated by setters/getters. My previous comment was more relevant to accessing state in other objects. https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock.h (right): https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:19: // N processes is an array/vector of N logical clocks, one clock per process A summary description and a link to Wikipedia might be more effective than an excerpt. We don't have N>2 processes, and the Lamport timestamp reference might be too esoteric for a code comment. https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:21: // In the particular case where N=2 (Client and Engine). So we can optimize to Sentence doesn't parse. https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:24: public: Add a type alias for Revision (uint32_t) and use that consistently. https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:33: // * EqualTo: Both revisions are equals. equals = equal What about "the same", so we don't have a circular reference with the word "equal"? https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:39: // Merges two vector clocks, this is useful/used after we merge the client The "why" should be more generalized, so that we promote orthogonal design in our foundation classes. https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:43: // Increments one to the local_revision_. This is used when something changes Active voice: "Increments local_revision_ by one." https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:44: // in the local host like setting a property or applying a change set. local state https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:44: // in the local host like setting a property or applying a change set. Does applying changesets modify the local revision? I thought it would only modify the remote revision? https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:48: void set_local_revision(int32_t local_revision) { Why will we need to support a setter? https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock_unittest.cc (right): https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock_unittest.cc:92: void CommutativeMergeTest(const VectorClock& v1, Not a test in its own right - "CheckCumulativeMerge"? Make helper function a protected member of VectorClockTest. https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock_unittest.cc:106: TEST_F(VectorClockTest, MergeEqualSmaller) { suggestion: EqualLocalSmallerRemote, etc.? ...
https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock.cc (right): https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.cc:19: DCHECK(local_revision() >= other.local_revision()); On 2016/09/27 21:36:09, Kevin M wrote: > add blank line below dchecks Done. https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.cc:39: DCHECK(false) << "Local revision should always be greater or equal."; On 2016/09/27 21:36:09, Kevin M wrote: > * Already checked in the top of CompareTo. > * No need to add explanation; that DCHECK body is self explanatory. > * DCHECK(false) should be DLOG(FATAL) Done. https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.cc:53: set_local_revision(local_revision() + 1); On 2016/09/27 21:36:09, Kevin M wrote: > Accesses to members of |this| needn't be mediated by setters/getters. My > previous comment was more relevant to accessing state in other objects. Done. https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock.h (right): https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:19: // N processes is an array/vector of N logical clocks, one clock per process On 2016/09/27 21:36:09, Kevin M wrote: > A summary description and a link to Wikipedia might be more effective than an > excerpt. We don't have N>2 processes, and the Lamport timestamp reference might > be too esoteric for a code comment. Done. https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:21: // In the particular case where N=2 (Client and Engine). So we can optimize to On 2016/09/27 21:36:09, Kevin M wrote: > Sentence doesn't parse. Done. https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:24: public: On 2016/09/27 21:36:09, Kevin M wrote: > Add a type alias for Revision (uint32_t) and use that consistently. Done. https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:33: // * EqualTo: Both revisions are equals. On 2016/09/27 21:36:09, Kevin M wrote: > equals = equal > > What about "the same", so we don't have a circular reference with the word > "equal"? Done. https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:39: // Merges two vector clocks, this is useful/used after we merge the client On 2016/09/27 21:36:09, Kevin M wrote: > The "why" should be more generalized, so that we promote orthogonal design in > our foundation classes. Done. https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:43: // Increments one to the local_revision_. This is used when something changes On 2016/09/27 21:36:09, Kevin M wrote: > Active voice: "Increments local_revision_ by one." Done. https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:44: // in the local host like setting a property or applying a change set. On 2016/09/27 21:36:09, Kevin M wrote: > Does applying changesets modify the local revision? I thought it would only > modify the remote revision? I think if we follow the vector clock definition yes: "Each time a process receives a message, it increments its own logical clock in the vector by one and updates each element in its vector by taking the maximum of the value in its own vector clock and the value in the vector in the received message (for every element)." https://en.wikipedia.org/wiki/Vector_clock https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:44: // in the local host like setting a property or applying a change set. On 2016/09/27 21:36:09, Kevin M wrote: > local state Done. https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:48: void set_local_revision(int32_t local_revision) { On 2016/09/27 21:36:09, Kevin M wrote: > Why will we need to support a setter? Done. https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock_unittest.cc (right): https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock_unittest.cc:92: void CommutativeMergeTest(const VectorClock& v1, On 2016/09/27 21:36:09, Kevin M wrote: > Not a test in its own right - "CheckCumulativeMerge"? > > Make helper function a protected member of VectorClockTest. Done. https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock_unittest.cc:106: TEST_F(VectorClockTest, MergeEqualSmaller) { On 2016/09/27 21:36:09, Kevin M wrote: > suggestion: EqualLocalSmallerRemote, etc.? > ... Done.
lgtm % nits https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock.h (right): https://codereview.chromium.org/2372903002/diff/100001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:44: // in the local host like setting a property or applying a change set. On 2016/09/27 23:23:11, scf wrote: > On 2016/09/27 21:36:09, Kevin M wrote: > > Does applying changesets modify the local revision? I thought it would only > > modify the remote revision? > > I think if we follow the vector clock definition yes: "Each time a process > receives a message, it increments its own logical clock in the vector by one and > updates each element in its vector by taking the maximum of the value in its own > vector clock and the value in the vector in the received message (for every > element)." > > > https://en.wikipedia.org/wiki/Vector_clock > Wouldn't this necessarily be a conflicting state, then? Maybe "process receives a message" is distributed system-ese for "a process' state is mutated"? https://codereview.chromium.org/2372903002/diff/140001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock.cc (right): https://codereview.chromium.org/2372903002/diff/140001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.cc:13: VectorClock::VectorClock() : local_revision_(0), remote_revision_(0) {} Put the zeroes in the .h as inline initializers https://codereview.chromium.org/2372903002/diff/140001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.cc:40: DLOG(FATAL) << "Local revision should always be greater or equal."; This state is pretty messed up, right? Maybe we should just LOG(FATAL) and crash outright on release builds. Debug builds will never hit this line; the DCHECK on line 19 will abort execution. https://codereview.chromium.org/2372903002/diff/140001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock.h (right): https://codereview.chromium.org/2372903002/diff/140001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:14: // From wikipedia: I meant, can we summarize what this does for Blimp/Helium? https://codereview.chromium.org/2372903002/diff/140001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:21: typedef int32_t Revision; uint32_t? https://codereview.chromium.org/2372903002/diff/140001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:39: // Merges two vector clocks, this function should be used at synchronization Merges two vector clocks. (period) This... https://codereview.chromium.org/2372903002/diff/140001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:50: } Add newline after the local_ methods; or separate each method with a newline.
Also, before submitting:
* This CL should not be "private". I don't know if you can demote this from
private to public or if we would need to spin up a new CL.
* The CL description needs more meat. You can use paragraphs or bullet points,
but since this goes in the monolithic Chromium Git log, the CLs need sufficient
context that they don't confuse random folks elsewhere on the project. You can
look at mine or Wez's CL history for some examples ("git log --author=kmarshall"
or "git log --author=wez")
* CL description is missing a BUG=<bug ID> entry.
https://codereview.chromium.org/2372903002/diff/140001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock.cc (right): https://codereview.chromium.org/2372903002/diff/140001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.cc:13: VectorClock::VectorClock() : local_revision_(0), remote_revision_(0) {} On 2016/09/28 01:23:15, Kevin M wrote: > Put the zeroes in the .h as inline initializers Done. https://codereview.chromium.org/2372903002/diff/140001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.cc:40: DLOG(FATAL) << "Local revision should always be greater or equal."; On 2016/09/28 01:23:15, Kevin M wrote: > This state is pretty messed up, right? Maybe we should just LOG(FATAL) and crash > outright on release builds. Debug builds will never hit this line; the DCHECK on > line 19 will abort execution. Done. https://codereview.chromium.org/2372903002/diff/140001/blimp/net/helium/vecto... File blimp/net/helium/vector_clock.h (right): https://codereview.chromium.org/2372903002/diff/140001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:14: // From wikipedia: On 2016/09/28 01:23:15, Kevin M wrote: > I meant, can we summarize what this does for Blimp/Helium? Done. https://codereview.chromium.org/2372903002/diff/140001/blimp/net/helium/vecto... blimp/net/helium/vector_clock.h:21: typedef int32_t Revision; On 2016/09/28 01:23:15, Kevin M wrote: > uint32_t? Done.
The CQ bit was checked by scf@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kmarshall@chromium.org Link to the patchset: https://codereview.chromium.org/2372903002/#ps160001 (title: "addressing changes")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2369323003 Patch 1). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by scf@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
CLs for remote refs other than refs/pending/heads/master must contain NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them
Description was changed from ========== Initial implementation of vector clock BUG= ========== to ========== Helium: Initial Implementation of vector clock Adding initial interface with methods to compare, merge two vectors and increment a local version of a vector clock. BUG=650461 ==========
The CQ bit was checked by scf@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kmarshall@chromium.org Link to the patchset: https://codereview.chromium.org/2372903002/#ps180001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by scf@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kmarshall@chromium.org Link to the patchset: https://codereview.chromium.org/2372903002/#ps200001 (title: "Fixing CQ issue regarding net export")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Helium: Initial Implementation of vector clock Adding initial interface with methods to compare, merge two vectors and increment a local version of a vector clock. BUG=650461 ========== to ========== Helium: Initial Implementation of vector clock Adding initial interface with methods to compare, merge two vectors and increment a local version of a vector clock. BUG=650461 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Helium: Initial Implementation of vector clock Adding initial interface with methods to compare, merge two vectors and increment a local version of a vector clock. BUG=650461 ========== to ========== Helium: Initial Implementation of vector clock Adding initial interface with methods to compare, merge two vectors and increment a local version of a vector clock. BUG=650461 Committed: https://crrev.com/4debff3c473712310b30ac84924f322ed2a2d7ed Cr-Commit-Position: refs/heads/master@{#421573} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/4debff3c473712310b30ac84924f322ed2a2d7ed Cr-Commit-Position: refs/heads/master@{#421573} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
