-
Notifications
You must be signed in to change notification settings - Fork 11
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
Added the test scripts for resumption #2117
base: main
Are you sure you want to change the base?
Conversation
|
Does your PR have changes that can cause upgrade issues?
|
""" | ||
Runs the yb-voyager command with support for resumption testing. | ||
""" | ||
for attempt in range(1, resumption['max_restarts'] + 1): |
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.
let's get/define all the configs in the beginning. It will make it easier to understand what all configuration options are involved.
max_restarts = resumption['max_restarts']
min_interrupt_seconds = resumption['min_interrupt_seconds']
...
if not output: # Exit if output is empty (end of process output) | ||
break | ||
full_output += output | ||
if time.time() - start_time > 5: |
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.
why break ? what is 5? seconds? minutes?
# Final import retry logic | ||
print("\n--- Final attempt to complete the import ---") | ||
|
||
for _ in range(2): |
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.
Why 2 attempts finally?
try: | ||
print("\nVoyager command output:") | ||
|
||
process = subprocess.Popen( |
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.
nit: separate function for starting command (can be called in above for-loop as well)
) | ||
|
||
# Capture and print output | ||
for line in iter(process.stdout.readline, ''): |
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.
in the above for-loop, we're reading both stderr and stdout, here we're only reading stdout. Any particular reason? Would be good to be consistent here (call a common function that captures stdout/stderr)
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.
Also till when will you keep reading? How long will the loop run?
for line in iter(process.stderr.readline, ''): | ||
print(line.strip()) | ||
sys.stdout.flush() | ||
time.sleep(30) |
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.
why sleep?
print("Final import failed after 2 attempts.") | ||
sys.exit(1) | ||
|
||
def validate_row_counts(row_count, export_dir): |
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.
note for future: you can create a common python file that has such helper
functions.
@@ -0,0 +1,133 @@ | |||
#!/bin/bash |
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.
Assuming that the ONLY change here is that you're specifying ROW_COUNT and essentially making generate_series dynamic.
schema2.Case_Sensitive_Table: 5000000 | ||
schema2.case: 5000000 | ||
schema2.Table: 5000000 | ||
public.boston: 2500000 |
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.
where is the code that generates data for all these other tables boston/cust/emp/etc? I only see code for table/case/Case_Sensitive_Table
Describe the changes in this pull request
Describe if there are any user-facing changes
N/A
How was this pull request tested?
Made the changes to the Jenkins pipeline as well.