-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: log rows and values in DefaultMismatchDetector #129
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @prawilny)
bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/utils/mirroringmetrics/MirroringMetricsRecorder.java, line 83 at r1 (raw file):
} public void recordMatchingReads(HBaseOperation operation, int numberOfMatchingReads) {
I'd unify the name with other record* methods in this file, e.g. recordReadMatches
.
bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/utils/mirroringmetrics/MirroringSpanConstants.java, line 65 at r1 (raw file):
"com/google/cloud/bigtable/mirroring/mismatch/write", "Count of write mismatches.", "1"); public static final MeasureLong MATCHING_READS =
Don't you need to define a View with this Measure? Take a look at usages of other Measures, you'll see that they are used in ...MetricViews.java file.
bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/utils/mirroringmetrics/MirroringSpanConstants.java, line 67 at r1 (raw file):
public static final MeasureLong MATCHING_READS = MeasureLong.create( "com/google/cloud/bigtable/mirroring/matching_reads",
I'd change the naming a bit. In fact, write mismatch
is not the best name I could have invented, it reports secondary write errors, not any mismatches :/
WDYT about changing com/google/cloud/bigtable/mirroring/mismatch/read
into com/google/cloud/bigtable/mirroring/read_verification/mismatches
, making this one .../read_verification/matches
, and creating a PR for renaming WRITE_MISMATCHES
into SECONDARY_WRITE_ERROR
?
bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/verification/DefaultMismatchDetector.java, line 63 at r1 (raw file):
"existsAll(row=%s) mismatch: (%b, %b)", hex(request.get(i).getRow()), primary[i], secondary[i]); ++mismatches;
Why are you using pre-incrementation?
bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/verification/DefaultMismatchDetector.java, line 168 at r1 (raw file):
} } if (errors != maxLength) {
Took me a while to be sure what is going on here, can we just make a separate variable correctReads
, increment it explicitly and report it if it is > 0
?
bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/verification/DefaultMismatchDetector.java, line 177 at r1 (raw file):
private List<String> listOfHexRows(List<Get> gets) { List<String> out = new ArrayList<>(gets.size());
We can make this computation lazy in the same way as hex()
.
bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/verification/DefaultMismatchDetector.java, line 189 at r1 (raw file):
}; private static String hex(final byte[] bytes) {
We can easily make this computation lazy making this method return an object, that will perform the hexlification when toString()
is called on it.
return new Object() {
@Override public String toString() {
/* body of this hex method */
}
}
2e6b192
to
6522048
Compare
eb38df3
to
5d5f1b6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 5 files reviewed, 7 unresolved discussions (waiting on @mwalkiewicz)
bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/utils/mirroringmetrics/MirroringMetricsRecorder.java, line 83 at r1 (raw file):
Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…
I'd unify the name with other record* methods in this file, e.g.
recordReadMatches
.
Done.
bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/utils/mirroringmetrics/MirroringSpanConstants.java, line 65 at r1 (raw file):
Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…
Don't you need to define a View with this Measure? Take a look at usages of other Measures, you'll see that they are used in ...MetricViews.java file.
I do. Didn't notice it earlier, though.
Done.
bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/utils/mirroringmetrics/MirroringSpanConstants.java, line 67 at r1 (raw file):
Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…
I'd change the naming a bit. In fact,
write mismatch
is not the best name I could have invented, it reports secondary write errors, not any mismatches :/WDYT about changing
com/google/cloud/bigtable/mirroring/mismatch/read
intocom/google/cloud/bigtable/mirroring/read_verification/mismatches
, making this one.../read_verification/matches
, and creating a PR for renamingWRITE_MISMATCHES
intoSECONDARY_WRITE_ERROR
?
Name change of read metrics done.
Also created a PR for WRITE_MISMATCHES -> SECONDARY_WRITE_ERRORS.
bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/verification/DefaultMismatchDetector.java, line 63 at r1 (raw file):
Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…
Why are you using pre-incrementation?
I must've felt like microoptimizing (stupid, I know).
Changed to post-incrementation.
bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/verification/DefaultMismatchDetector.java, line 168 at r1 (raw file):
Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…
Took me a while to be sure what is going on here, can we just make a separate variable
correctReads
, increment it explicitly and report it if it is> 0
?
Done.
bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/verification/DefaultMismatchDetector.java, line 177 at r1 (raw file):
Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…
We can make this computation lazy in the same way as
hex()
.
Done.
bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/verification/DefaultMismatchDetector.java, line 189 at r1 (raw file):
Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…
We can easily make this computation lazy making this method return an object, that will perform the hexlification when
toString()
is called on it.return new Object() { @Override public String toString() { /* body of this hex method */ } }
Done.
5d5f1b6
to
901a0a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 5 files at r2, all commit messages.
Reviewable status: 1 of 5 files reviewed, 8 unresolved discussions (waiting on @mwalkiewicz and @prawilny)
bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/utils/Hex.java, line 19 at r2 (raw file):
// Adapted from Apache Common Codec's Hex public class Hex {
This name doesn't describe the purpose of this class. What do you think about LazyBytesHexlifier
? It still won't be obvious why is it needed, thus a comment describing where it is used will be appropriate.
bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/utils/Hex.java, line 26 at r2 (raw file):
private final byte[] bytes; public Hex(byte[] bytes) {
We definitely want to limit length of produced string. We should add a parameter with max length of the output string.
bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/verification/DefaultMismatchDetector.java, line 177 at r1 (raw file):
Previously, prawilny (Adam Czajkowski) wrote…
Done.
So now we are printing a list of Hex
objects, I don't think that it is obvious that it will work as expected and we should test that.
OTOH I thought about changing returning a object HexList
instead of List<Hex>
which would lazily call for () { out.add(hex(row)); } return out;
.
Hmm, are you sure that logging lists works as expected and will print [item1, item2]
etc? There are no tests for that (and I do not even know how to test it :D maybe we should inject our own logger implementation to be used by the logging facade and verify if it was written? or, just test it manually to be sure and assume that it is obvious that it works :D )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 5 files reviewed, 8 unresolved discussions (waiting on @mwalkiewicz and @prawilny)
bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/utils/Hex.java, line 19 at r2 (raw file):
Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…
This name doesn't describe the purpose of this class. What do you think about
LazyBytesHexlifier
? It still won't be obvious why is it needed, thus a comment describing where it is used will be appropriate.
Well, this class is kinda obscure, maybe it'd be wise to hide it in MismatchDetector as an static inner class, wdyt?
bfd3c32
to
286fe15
Compare
286fe15
to
3403c8f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dismissed @mwalkiewicz from 4 discussions.
Reviewable status: 0 of 11 files reviewed, 4 unresolved discussions (waiting on @mwalkiewicz)
bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/verification/DefaultMismatchDetector.java, line 177 at r1 (raw file):
Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…
So now we are printing a list of
Hex
objects, I don't think that it is obvious that it will work as expected and we should test that.OTOH I thought about changing returning a object
HexList
instead ofList<Hex>
which would lazily callfor () { out.add(hex(row)); } return out;
.Hmm, are you sure that logging lists works as expected and will print
[item1, item2]
etc? There are no tests for that (and I do not even know how to test it :D maybe we should inject our own logger implementation to be used by the logging facade and verify if it was written? or, just test it manually to be sure and assume that it is obvious that it works :D )
Added a test for it (with simple string comparison).
bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/utils/Hex.java, line 19 at r2 (raw file):
Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…
Well, this class is kinda obscure, maybe it'd be wise to hide it in MismatchDetector as an static inner class, wdyt?
I think it was extracted into a standalone class in order to accomodate some logging done in MirroringScanner#LastUnmatched, but I'll probably move LastUnmatched to MismatchDetector so I think it'll be fine as a nested class of DefaultMismatchDetector.
Done.
bigtable-hbase-mirroring-client-1.x-parent/bigtable-hbase-mirroring-client-1.x/src/main/java/com/google/cloud/bigtable/mirroring/hbase1_x/utils/Hex.java, line 26 at r2 (raw file):
Previously, mwalkiewicz (Mateusz Walkiewicz) wrote…
We definitely want to limit length of produced string. We should add a parameter with max length of the output string.
Done. Also when limited it shows n/2 first bytes and n/2 last bytes (separated by "...").
… review comments (googleapis#3347) * chore: revert review comments * feat: add MirroringOperationException exception markers (#125) * feat: concurrent writes in MirroringBufferedMutator (#80) * refactor: implement multiple argument operations on MirroringAsyncTable with specific operations rather than batch() (#75) * feat: implement MirroringAsyncTable#getName() (#132) * feat: use Logger rather than stdout in DefaultMismatchDetector (#128) * feat: synchronous writes (#88) * fix: implement heapSize method for RowCell (#111) * feat: FlowController accounts for memory usage (#137) * refactor: remove Configuration as a base of MirroringConfiguration (#127) * feat: MirroringAsyncBufferedMutator (#81) * refactor: rename WRITE_MISMATCH to SECONDARY_WRITE_ERROR (#138) * fix: BufferedMutator close() waits for all secondary flushes to finish (#110) * feat: 2.x reads sampling (#114) * refactor: make MirroringResultScanner synchronize on itself rather than MirroringTable (#134) * ConcurrentBufferedMutator integration tests (#135) * feat: add synchronous MirroringConnection to 2.x (#109) * fix: MirroringConnection in 2.x failed to compile (#139) * fix: fix BufferedMutator ITs (#140) * feat: run 1.x integration tests on MirroringConnection etc. from 2.x (#108) * feat: 2.x - rewrite Increment and Append as Put in batch (#116) * fix: fix build (#142) * refactor: minor fixes after review (#117) * feat: MirroringAsyncTable#getScanner() (#58) * test: 2.x integration tests (#112) * feat: implement MirroringAsyncBufferedMutatorBuilder (#144) * feat: log rows and values in DefaultMismatchDetector (#129) * fix: ITs - add expected parameter to MismatchDetectors (#153) * fix: force Append and Increment to return results and discard that result before returning it to user (#136) * fix: review fixes in utils * fix: review fixes in BufferedMutator * fix: review fixes in Faillog * fix: fixed reference counting * fix: review fixes in FlowController * fix: review fixes in metrics * fix: review fixes in verification * fix: Review fixes in MirroringTable * fix: review fixes in HBase 2.x client * fix: fixes in ITs * feat: MirrorinAsyncTable: scan(), scanAll() (#131) * fix: review fixes in tests * feat: MirroringConnection: timeout in close() and abort() (#133) * feat: better mismatch detection of scan results (#130) * feat: quickstart (#105) * fix: 2.x scan ITs (#158) * fix: DefaultMismatchDetector tests (#157) * fix: ConcurrentBufferedMutator waits for both flushes to finish before closing (#161) * fix: additional minor fixes after review (#163) * fix: BufferedMutator review fixes (#164) - Simplify #flush(). - Add javadocs. - (sequential) Fix flush() exception handling. - (sequential) Move error handling to a separate inner class. * fix: PR fixes * fix: report zeroed error metrics after successful operations * fix: prepend MismatchDetectorCounter with Test to better reflect its purpose * feat: Client-side timestamping (#165) * fix: reduce timeout in TestBlocking to make the tests run faster * fix: asyncClose -> closePrimaryAndScheduleSecondaryClose * fix: remove unused Batcher#throwBatchDataExceptionIfPresent * fix: remove unused Comparators#compareRows * fix: extract failedReads from MatchingSuccessfulReadsResults to reduce confusion * feat: remove unused MirroringTracer from FailedMutationLogger * fix: MirroringAsyncBufferedMutator - test if failed mutation is passed to secondary write error consumer * fix: TestMirroringAsyncTableInputModification typo fix * fix: describe user flush() in Buffered Mutator in quickstart * fix: MirroringBufferedMutator - move flush threshold from BufferedMutations to FlushSerializer * refactor: MirroringBufferedMutator#close() - use AccumulatedExceptions insteand of List<Exception> * BufferedMutator - add close timeout * AsyncBufferedMutator - add close timeout * fix: remove stale addSecondaryMutation comment * fix: add a comment that addSecondaryMutation handles failed writes * fix: unify implementations of flushBufferedMutatorBeforeClosing * fix: BufferedMutator - throw exceptions on close * fix: BufferedMutator - add comment explaining that chain of flush operations is created * fix: BufferedMutator - clarify comments * fix: Concurrent BufferedMutator - fix throwFlushExceptionIfAvailable * fix: explain why flush is called in Sequential BufferedMutator test * fix: TestConcurrentMirroringBufferedMutator - make waiting for calls explicit * refactor: BufferedMutator rename scheduleFlushAll() to scheduleFlush() * refactor: make FlushSerializer non-static * fix: BufferedMutator - use HierarchicalReferenceCounter * feat: Add MirroringConnection constructor taking MirroringConfiguration * refactor: move releaseReservations to finally * fix: use less convoluted example in lastFlushFutures description * fix: merge small Timeestamper files into a single file * fix: add a comment explaining which exceptions are forwarded to the user and why in SequentialMirroringBufferedMutator * fix: use UnsupportedOperationException instead of RuntimeException when forbidden mutation type is encountered * fix: add comment explaining why batch is complicated * fix: add a TODO to implement point writes without batch Co-authored-by: Mateusz Walkiewicz <mwalkiewicz@unoperate.com> Co-authored-by: Adam Czajkowski <prawilny@unoperate.com> Co-authored-by: Kajetan Boroszko <kajetan@unoperate.com>
… review comments (googleapis#3347) * chore: revert review comments * feat: add MirroringOperationException exception markers (#125) * feat: concurrent writes in MirroringBufferedMutator (#80) * refactor: implement multiple argument operations on MirroringAsyncTable with specific operations rather than batch() (#75) * feat: implement MirroringAsyncTable#getName() (#132) * feat: use Logger rather than stdout in DefaultMismatchDetector (#128) * feat: synchronous writes (#88) * fix: implement heapSize method for RowCell (#111) * feat: FlowController accounts for memory usage (#137) * refactor: remove Configuration as a base of MirroringConfiguration (#127) * feat: MirroringAsyncBufferedMutator (#81) * refactor: rename WRITE_MISMATCH to SECONDARY_WRITE_ERROR (#138) * fix: BufferedMutator close() waits for all secondary flushes to finish (#110) * feat: 2.x reads sampling (#114) * refactor: make MirroringResultScanner synchronize on itself rather than MirroringTable (#134) * ConcurrentBufferedMutator integration tests (#135) * feat: add synchronous MirroringConnection to 2.x (#109) * fix: MirroringConnection in 2.x failed to compile (#139) * fix: fix BufferedMutator ITs (#140) * feat: run 1.x integration tests on MirroringConnection etc. from 2.x (#108) * feat: 2.x - rewrite Increment and Append as Put in batch (#116) * fix: fix build (#142) * refactor: minor fixes after review (#117) * feat: MirroringAsyncTable#getScanner() (#58) * test: 2.x integration tests (#112) * feat: implement MirroringAsyncBufferedMutatorBuilder (#144) * feat: log rows and values in DefaultMismatchDetector (#129) * fix: ITs - add expected parameter to MismatchDetectors (#153) * fix: force Append and Increment to return results and discard that result before returning it to user (#136) * fix: review fixes in utils * fix: review fixes in BufferedMutator * fix: review fixes in Faillog * fix: fixed reference counting * fix: review fixes in FlowController * fix: review fixes in metrics * fix: review fixes in verification * fix: Review fixes in MirroringTable * fix: review fixes in HBase 2.x client * fix: fixes in ITs * feat: MirrorinAsyncTable: scan(), scanAll() (#131) * fix: review fixes in tests * feat: MirroringConnection: timeout in close() and abort() (#133) * feat: better mismatch detection of scan results (#130) * feat: quickstart (#105) * fix: 2.x scan ITs (#158) * fix: DefaultMismatchDetector tests (#157) * fix: ConcurrentBufferedMutator waits for both flushes to finish before closing (#161) * fix: additional minor fixes after review (#163) * fix: BufferedMutator review fixes (#164) - Simplify #flush(). - Add javadocs. - (sequential) Fix flush() exception handling. - (sequential) Move error handling to a separate inner class. * fix: PR fixes * fix: report zeroed error metrics after successful operations * fix: prepend MismatchDetectorCounter with Test to better reflect its purpose * feat: Client-side timestamping (#165) * fix: reduce timeout in TestBlocking to make the tests run faster * fix: asyncClose -> closePrimaryAndScheduleSecondaryClose * fix: remove unused Batcher#throwBatchDataExceptionIfPresent * fix: remove unused Comparators#compareRows * fix: extract failedReads from MatchingSuccessfulReadsResults to reduce confusion * feat: remove unused MirroringTracer from FailedMutationLogger * fix: MirroringAsyncBufferedMutator - test if failed mutation is passed to secondary write error consumer * fix: TestMirroringAsyncTableInputModification typo fix * fix: describe user flush() in Buffered Mutator in quickstart * fix: MirroringBufferedMutator - move flush threshold from BufferedMutations to FlushSerializer * refactor: MirroringBufferedMutator#close() - use AccumulatedExceptions insteand of List<Exception> * BufferedMutator - add close timeout * AsyncBufferedMutator - add close timeout * fix: remove stale addSecondaryMutation comment * fix: add a comment that addSecondaryMutation handles failed writes * fix: unify implementations of flushBufferedMutatorBeforeClosing * fix: BufferedMutator - throw exceptions on close * fix: BufferedMutator - add comment explaining that chain of flush operations is created * fix: BufferedMutator - clarify comments * fix: Concurrent BufferedMutator - fix throwFlushExceptionIfAvailable * fix: explain why flush is called in Sequential BufferedMutator test * fix: TestConcurrentMirroringBufferedMutator - make waiting for calls explicit * refactor: BufferedMutator rename scheduleFlushAll() to scheduleFlush() * refactor: make FlushSerializer non-static * fix: BufferedMutator - use HierarchicalReferenceCounter * feat: Add MirroringConnection constructor taking MirroringConfiguration * refactor: move releaseReservations to finally * fix: use less convoluted example in lastFlushFutures description * fix: merge small Timeestamper files into a single file * fix: add a comment explaining which exceptions are forwarded to the user and why in SequentialMirroringBufferedMutator * fix: use UnsupportedOperationException instead of RuntimeException when forbidden mutation type is encountered * fix: add comment explaining why batch is complicated * fix: add a TODO to implement point writes without batch Co-authored-by: Mateusz Walkiewicz <mwalkiewicz@unoperate.com> Co-authored-by: Adam Czajkowski <prawilny@unoperate.com> Co-authored-by: Kajetan Boroszko <kajetan@unoperate.com>
This change is