-
Notifications
You must be signed in to change notification settings - Fork 18
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
fixing progress counter bug #197
Conversation
@@ -6,3 +6,6 @@ spark.cdm.schema.target.keyspaceTable target.regression_performance | |||
|
|||
spark.cdm.perfops.numParts 32 | |||
spark.cdm.perfops.batchSize 1 | |||
|
|||
spark.cdm.perfops.printStatsAfter 450 |
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.
LGTM. Let's please coordinate with docs crew to publish this at https://docs.datastax.com/en/astra-serverless/docs/migrate/cassandra-data-migrator.html page
src/test/java/com/datastax/cdm/properties/PropertyHelperTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/datastax/cdm/properties/PropertyHelperTest.java
Outdated
Show resolved
Hide resolved
|
||
public void printProgress() { | ||
printProgress(false); | ||
printProgress(true); |
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.
I don't understand what we're doing in here by calling both true
and false
?
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.
Yeah that's a bit of an interesting choice, not entirely happy with it. false
is for the thread itself, and I was trying to hide this messiness from the rest of the application. There are two kinds of progress counts, the existing printStatsAfter
gives feedback as to how we are progressing within the global dataset, and then the new (and noisier) printStatsPerPart
which prints an entry as each part completes.
public void printProgress(boolean global) {
if (global) {
if (shouldPrintGlobalProgress()) {
printAndLogProgress("Progress Counts: ", true);
}
} else {
if (printPerThread) {
printAndLogProgress("Thread Counts: ", false);
}
}
}
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.
This should be just 2 separate methods (printGlobalProgress & printThreadProgess) to make it cleaner.
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.
OR remove the printProgress(boolean global)
method completely & refactor the printProgress()
method to something like below
public void printProgress() {
if (printPerThread) {
printAndLogProgress("Thread Counts: ", false);
}
if (shouldPrintGlobalProgress()) {
printAndLogProgress("Progress Counts: ", true);
}
}
Any idea why we're receiving these |
@msmygit the |
|
||
public void printProgress() { | ||
printProgress(false); | ||
printProgress(true); |
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.
This should be just 2 separate methods (printGlobalProgress & printThreadProgess) to make it cleaner.
|
||
public void printProgress() { | ||
printProgress(false); | ||
printProgress(true); |
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.
OR remove the printProgress(boolean global)
method completely & refactor the printProgress()
method to something like below
public void printProgress() {
if (printPerThread) {
printAndLogProgress("Thread Counts: ", false);
}
if (shouldPrintGlobalProgress()) {
printAndLogProgress("Progress Counts: ", true);
}
}
I've encorporated the review comments and pushed the changes. Merging here. |
JobCounter
Which issue(s) this PR fixes:
Fixes #196
Checklist: