New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

storage: fail commands outside the replica GC threshold #6992

Merged
merged 1 commit into from Jun 4, 2016

Conversation

Projects
None yet
3 participants
@mjibson
Member

mjibson commented Jun 1, 2016

This change is Reviewable

@mjibson

This comment has been minimized.

Show comment
Hide comment
@mjibson

mjibson Jun 1, 2016

Member

This is blocked on a decision by #6985.

Member

mjibson commented Jun 1, 2016

This is blocked on a decision by #6985.

@bdarnell

This comment has been minimized.

Show comment
Hide comment
@bdarnell

bdarnell Jun 3, 2016

Member

#6985 was decided in favor of allowing the inconsistency, on the grounds that it is self-healing. This change could introduce non-self-healing inconsistencies. However, queries near the GC cutoff would be so buggy prior to this change that I'm not sure if we care.

LGTM

Previously, mjibson (Matt Jibson) wrote…

This is blocked on a decision by #6985.


Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


storage/replica_test.go, line 5640 [r1] (raw file):

  // Verify a later Get works.
  if _, err := tc.SendWrappedWith(roachpb.Header{
      Timestamp: makeTS(3, 0),

Put the timestamp(3,0) in a variable to make sure it's the same both times it's used.


Comments from Reviewable

Member

bdarnell commented Jun 3, 2016

#6985 was decided in favor of allowing the inconsistency, on the grounds that it is self-healing. This change could introduce non-self-healing inconsistencies. However, queries near the GC cutoff would be so buggy prior to this change that I'm not sure if we care.

LGTM

Previously, mjibson (Matt Jibson) wrote…

This is blocked on a decision by #6985.


Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


storage/replica_test.go, line 5640 [r1] (raw file):

  // Verify a later Get works.
  if _, err := tc.SendWrappedWith(roachpb.Header{
      Timestamp: makeTS(3, 0),

Put the timestamp(3,0) in a variable to make sure it's the same both times it's used.


Comments from Reviewable

@tschottdorf

This comment has been minimized.

Show comment
Hide comment
@tschottdorf

tschottdorf Jun 3, 2016

Member

:lgtm:

Previously, bdarnell (Ben Darnell) wrote…

#6985 was decided in favor of allowing the inconsistency, on the grounds that it is self-healing. This change could introduce non-self-healing inconsistencies. However, queries near the GC cutoff would be so buggy prior to this change that I'm not sure if we care.

LGTM


Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


storage/replica_test.go, line 5642 [r1] (raw file):

      Timestamp: makeTS(3, 0),
  }, &gArgs); err != nil {
      t.Fatalf("could not get data: %s", err)

Add similar tests for a ConditionalPut. I expect this to work (since executeBatch is called from executeWriteBatch), but it should be asserted.


Comments from Reviewable

Member

tschottdorf commented Jun 3, 2016

:lgtm:

Previously, bdarnell (Ben Darnell) wrote…

#6985 was decided in favor of allowing the inconsistency, on the grounds that it is self-healing. This change could introduce non-self-healing inconsistencies. However, queries near the GC cutoff would be so buggy prior to this change that I'm not sure if we care.

LGTM


Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


storage/replica_test.go, line 5642 [r1] (raw file):

      Timestamp: makeTS(3, 0),
  }, &gArgs); err != nil {
      t.Fatalf("could not get data: %s", err)

Add similar tests for a ConditionalPut. I expect this to work (since executeBatch is called from executeWriteBatch), but it should be asserted.


Comments from Reviewable

@tschottdorf

This comment has been minimized.

Show comment
Hide comment
@tschottdorf

tschottdorf Jun 3, 2016

Member

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


Comments from Reviewable

Member

tschottdorf commented Jun 3, 2016

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


Comments from Reviewable

@mjibson

This comment has been minimized.

Show comment
Hide comment
@mjibson

mjibson Jun 3, 2016

Member

Review status: 1 of 2 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.


storage/replica_test.go, line 5640 [r1] (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Put the timestamp(3,0) in a variable to make sure it's the same both times it's used.

Done.

storage/replica_test.go, line 5642 [r1] (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Add similar tests for a ConditionalPut. I expect this to work (since executeBatch is called from executeWriteBatch), but it should be asserted.

Done.

Comments from Reviewable

Member

mjibson commented Jun 3, 2016

Review status: 1 of 2 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.


storage/replica_test.go, line 5640 [r1] (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Put the timestamp(3,0) in a variable to make sure it's the same both times it's used.

Done.

storage/replica_test.go, line 5642 [r1] (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Add similar tests for a ConditionalPut. I expect this to work (since executeBatch is called from executeWriteBatch), but it should be asserted.

Done.

Comments from Reviewable

@tschottdorf

This comment has been minimized.

Show comment
Hide comment
@tschottdorf

tschottdorf Jun 3, 2016

Member

:lgtm:

Previously, tschottdorf (Tobias Schottdorf) wrote…

:lgtm:


Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


Comments from Reviewable

Member

tschottdorf commented Jun 3, 2016

:lgtm:

Previously, tschottdorf (Tobias Schottdorf) wrote…

:lgtm:


Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


Comments from Reviewable

@mjibson mjibson merged commit 081f91c into cockroachdb:master Jun 4, 2016

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
code-review/reviewable Review complete: 1 of 0 LGTMs obtained
Details
licence/cla Contributor License Agreement is signed.
Details

@mjibson mjibson deleted the mjibson:storage-time-replica branch Jun 4, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment