-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7135/
-----------------------------------------------------------

Review request for Sqoop.
Description
-------

Code review for SQOOP-604, see https://issues.apache.org/jira/browse/SQOOP-604

The solution in short: Using the already existing "checkpoint" feature of the direct (--direct) MySQL exports (the export process is restarted every X bytes written), extending it with a new config value that would simply make the thread sleep for X milliseconds at the checkbpoints. With low enough byte count limit this can be a simple yet powerful throttling mechanism.
Diffs
-----

  src/java/org/apache/sqoop/mapreduce/MySQLExportMapper.java a4e8b88

Diff: https://reviews.apache.org/r/7135/diff/
Testing
-------

Executing with different settings of sqoop.mysql.export.checkpoint.bytes and sqoop.mysql.export.sleep.ms:

33554432B / 0ms: Transferred 4.7579 MB in 8.7175 seconds (558.8826 KB/sec)
102400B / 500ms: Transferred 4.7579 MB in 35.7794 seconds (136.1698 KB/sec)
51200B / 500ms: Transferred 4.758 MB in 57.8675 seconds (84.1959 KB/sec)
51200B / 250ms: Transferred 4.7579 MB in 35.0293 seconds (139.0854 KB/sec)

I did not add unit tests yet and as it involves calling to Thread.sleep, I find testing this difficult. Unfortunately there is no "machine" or "environment" object that could be injected to these classes as mocks that could take care of time-related fixtures.
Thanks,

Zoltán Tóth-Czifra

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7135/
-----------------------------------------------------------

(Updated Oct. 1, 2012, 9:54 a.m.)
Review request for Sqoop.
Changes
-------

Added the requested change regarding mapred.task.timeout.
Description
-------

Code review for SQOOP-604, see https://issues.apache.org/jira/browse/SQOOP-604

The solution in short: Using the already existing "checkpoint" feature of the direct (--direct) MySQL exports (the export process is restarted every X bytes written), extending it with a new config value that would simply make the thread sleep for X milliseconds at the checkbpoints. With low enough byte count limit this can be a simple yet powerful throttling mechanism.
Diffs (updated)
-----

  src/java/org/apache/sqoop/mapreduce/MySQLExportMapper.java a4e8b88

Diff: https://reviews.apache.org/r/7135/diff/
Testing
-------

Executing with different settings of sqoop.mysql.export.checkpoint.bytes and sqoop.mysql.export.sleep.ms:

33554432B / 0ms: Transferred 4.7579 MB in 8.7175 seconds (558.8826 KB/sec)
102400B / 500ms: Transferred 4.7579 MB in 35.7794 seconds (136.1698 KB/sec)
51200B / 500ms: Transferred 4.758 MB in 57.8675 seconds (84.1959 KB/sec)
51200B / 250ms: Transferred 4.7579 MB in 35.0293 seconds (139.0854 KB/sec)

I did not add unit tests yet and as it involves calling to Thread.sleep, I find testing this difficult. Unfortunately there is no "machine" or "environment" object that could be injected to these classes as mocks that could take care of time-related fixtures.
Thanks,

Zoltán Tóth-Czifra

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7135/#review12058
-----------------------------------------------------------
Few Comments.
After corrections, please upload the new patch on JIRA as well Review board.

Thanks!
src/java/org/apache/sqoop/mapreduce/MySQLExportMapper.java
<https://reviews.apache.org/r/7135/#comment25704>

    Remove the trailing whitespace.

src/java/org/apache/sqoop/mapreduce/MySQLExportMapper.java
<https://reviews.apache.org/r/7135/#comment25712>

    I suppose this should be checkpointSleepMs?

src/java/org/apache/sqoop/mapreduce/MySQLExportMapper.java
<https://reviews.apache.org/r/7135/#comment25713>

    This whole block should refer to checkpointSleepMs rather checkpointDistInBytes.

src/java/org/apache/sqoop/mapreduce/MySQLExportMapper.java
<https://reviews.apache.org/r/7135/#comment25706>

    Seems like this line is more than 80 characters. Checkstyle will complain.
- Abhijeet Gaikwad
On Oct. 1, 2012, 9:54 a.m., Zoltán Tóth-Czifra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7135/
> -----------------------------------------------------------
>
> (Updated Oct. 1, 2012, 9:54 a.m.)
>
>
> Review request for Sqoop.
>
>
> Description
> -------
>
> Code review for SQOOP-604, see https://issues.apache.org/jira/browse/SQOOP-604
>
> The solution in short: Using the already existing "checkpoint" feature of the direct (--direct) MySQL exports (the export process is restarted every X bytes written), extending it with a new config value that would simply make the thread sleep for X milliseconds at the checkbpoints. With low enough byte count limit this can be a simple yet powerful throttling mechanism.
>
>
> Diffs
> -----
>
>   src/java/org/apache/sqoop/mapreduce/MySQLExportMapper.java a4e8b88
>
> Diff: https://reviews.apache.org/r/7135/diff/
>
>
> Testing
> -------
>
> Executing with different settings of sqoop.mysql.export.checkpoint.bytes and sqoop.mysql.export.sleep.ms:
>
> 33554432B / 0ms: Transferred 4.7579 MB in 8.7175 seconds (558.8826 KB/sec)
> 102400B / 500ms: Transferred 4.7579 MB in 35.7794 seconds (136.1698 KB/sec)
> 51200B / 500ms: Transferred 4.758 MB in 57.8675 seconds (84.1959 KB/sec)
> 51200B / 250ms: Transferred 4.7579 MB in 35.0293 seconds (139.0854 KB/sec)
>
> I did not add unit tests yet and as it involves calling to Thread.sleep, I find testing this difficult. Unfortunately there is no "machine" or "environment" object that could be injected to these classes as mocks that could take care of time-related fixtures.
>
>
> Thanks,
>
> Zoltán Tóth-Czifra
>
>

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7135/
-----------------------------------------------------------

(Updated Oct. 2, 2012, 10:08 a.m.)
Review request for Sqoop.
Changes
-------

You are right, I was in a hurry and here is the result. Anyways, I attach the fixed patch. Compiled with no checkstyle warnings.
Output of test:

2012-10-02 12:03:17,575 WARN com.cloudera.sqoop.mapreduce.MySQLExportMapper: Value for sqoop.mysql.export.sleep.ms has to be smaller than mapred.task.timeout
Description
-------

Code review for SQOOP-604, see https://issues.apache.org/jira/browse/SQOOP-604

The solution in short: Using the already existing "checkpoint" feature of the direct (--direct) MySQL exports (the export process is restarted every X bytes written), extending it with a new config value that would simply make the thread sleep for X milliseconds at the checkbpoints. With low enough byte count limit this can be a simple yet powerful throttling mechanism.
Diffs (updated)
-----

  src/java/org/apache/sqoop/mapreduce/MySQLExportMapper.java a4e8b88

Diff: https://reviews.apache.org/r/7135/diff/
Testing
-------

Executing with different settings of sqoop.mysql.export.checkpoint.bytes and sqoop.mysql.export.sleep.ms:

33554432B / 0ms: Transferred 4.7579 MB in 8.7175 seconds (558.8826 KB/sec)
102400B / 500ms: Transferred 4.7579 MB in 35.7794 seconds (136.1698 KB/sec)
51200B / 500ms: Transferred 4.758 MB in 57.8675 seconds (84.1959 KB/sec)
51200B / 250ms: Transferred 4.7579 MB in 35.0293 seconds (139.0854 KB/sec)

I did not add unit tests yet and as it involves calling to Thread.sleep, I find testing this difficult. Unfortunately there is no "machine" or "environment" object that could be injected to these classes as mocks that could take care of time-related fixtures.
Thanks,

Zoltán Tóth-Czifra

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7135/#review12099
-----------------------------------------------------------
Few more comments.
Run "ant checkstyle" before you next submit the patch. Also, attach the updated patch file on JIRA (SQOOP-604).
src/java/org/apache/sqoop/mapreduce/MySQLExportMapper.java
<https://reviews.apache.org/r/7135/#comment25780>

    Remove the extra space at the end of lines 339 and 340.

src/java/org/apache/sqoop/mapreduce/MySQLExportMapper.java
<https://reviews.apache.org/r/7135/#comment25781>

    You meant checkpointSleepMs?
- Abhijeet Gaikwad
On Oct. 2, 2012, 10:08 a.m., Zoltán Tóth-Czifra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7135/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2012, 10:08 a.m.)
>
>
> Review request for Sqoop.
>
>
> Description
> -------
>
> Code review for SQOOP-604, see https://issues.apache.org/jira/browse/SQOOP-604
>
> The solution in short: Using the already existing "checkpoint" feature of the direct (--direct) MySQL exports (the export process is restarted every X bytes written), extending it with a new config value that would simply make the thread sleep for X milliseconds at the checkbpoints. With low enough byte count limit this can be a simple yet powerful throttling mechanism.
>
>
> Diffs
> -----
>
>   src/java/org/apache/sqoop/mapreduce/MySQLExportMapper.java a4e8b88
>
> Diff: https://reviews.apache.org/r/7135/diff/
>
>
> Testing
> -------
>
> Executing with different settings of sqoop.mysql.export.checkpoint.bytes and sqoop.mysql.export.sleep.ms:
>
> 33554432B / 0ms: Transferred 4.7579 MB in 8.7175 seconds (558.8826 KB/sec)
> 102400B / 500ms: Transferred 4.7579 MB in 35.7794 seconds (136.1698 KB/sec)
> 51200B / 500ms: Transferred 4.758 MB in 57.8675 seconds (84.1959 KB/sec)
> 51200B / 250ms: Transferred 4.7579 MB in 35.0293 seconds (139.0854 KB/sec)
>
> I did not add unit tests yet and as it involves calling to Thread.sleep, I find testing this difficult. Unfortunately there is no "machine" or "environment" object that could be injected to these classes as mocks that could take care of time-related fixtures.
>
>
> Thanks,
>
> Zoltán Tóth-Czifra
>
>

> On Oct. 2, 2012, 3:45 p.m., Abhijeet Gaikwad wrote:
> > src/java/org/apache/sqoop/mapreduce/MySQLExportMapper.java, line 342
> > <https://reviews.apache.org/r/7135/diff/3/?file=172556#file172556line342>
> >
> >     You meant checkpointSleepMs?

Seems this is not yet resolved.
- Abhijeet
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7135/#review12099
-----------------------------------------------------------
On Oct. 2, 2012, 4:08 p.m., Zoltán Tóth-Czifra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7135/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2012, 4:08 p.m.)
>
>
> Review request for Sqoop.
>
>
> Description
> -------
>
> Code review for SQOOP-604, see https://issues.apache.org/jira/browse/SQOOP-604
>
> The solution in short: Using the already existing "checkpoint" feature of the direct (--direct) MySQL exports (the export process is restarted every X bytes written), extending it with a new config value that would simply make the thread sleep for X milliseconds at the checkbpoints. With low enough byte count limit this can be a simple yet powerful throttling mechanism.
>
>
> Diffs
> -----
>
>   src/java/org/apache/sqoop/mapreduce/MySQLExportMapper.java a4e8b88
>
> Diff: https://reviews.apache.org/r/7135/diff/
>
>
> Testing
> -------
>
> Executing with different settings of sqoop.mysql.export.checkpoint.bytes and sqoop.mysql.export.sleep.ms:
>
> 33554432B / 0ms: Transferred 4.7579 MB in 8.7175 seconds (558.8826 KB/sec)
> 102400B / 500ms: Transferred 4.7579 MB in 35.7794 seconds (136.1698 KB/sec)
> 51200B / 500ms: Transferred 4.758 MB in 57.8675 seconds (84.1959 KB/sec)
> 51200B / 250ms: Transferred 4.7579 MB in 35.0293 seconds (139.0854 KB/sec)
>
> I did not add unit tests yet and as it involves calling to Thread.sleep, I find testing this difficult. Unfortunately there is no "machine" or "environment" object that could be injected to these classes as mocks that could take care of time-related fixtures.
>
>
> Thanks,
>
> Zoltán Tóth-Czifra
>
>

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7135/
-----------------------------------------------------------

(Updated Oct. 2, 2012, 4:08 p.m.)
Review request for Sqoop.
Changes
-------

Sorry!
Description
-------

Code review for SQOOP-604, see https://issues.apache.org/jira/browse/SQOOP-604

The solution in short: Using the already existing "checkpoint" feature of the direct (--direct) MySQL exports (the export process is restarted every X bytes written), extending it with a new config value that would simply make the thread sleep for X milliseconds at the checkbpoints. With low enough byte count limit this can be a simple yet powerful throttling mechanism.
Diffs (updated)
-----

  src/java/org/apache/sqoop/mapreduce/MySQLExportMapper.java a4e8b88

Diff: https://reviews.apache.org/r/7135/diff/
Testing
-------

Executing with different settings of sqoop.mysql.export.checkpoint.bytes and sqoop.mysql.export.sleep.ms:

33554432B / 0ms: Transferred 4.7579 MB in 8.7175 seconds (558.8826 KB/sec)
102400B / 500ms: Transferred 4.7579 MB in 35.7794 seconds (136.1698 KB/sec)
51200B / 500ms: Transferred 4.758 MB in 57.8675 seconds (84.1959 KB/sec)
51200B / 250ms: Transferred 4.7579 MB in 35.0293 seconds (139.0854 KB/sec)

I did not add unit tests yet and as it involves calling to Thread.sleep, I find testing this difficult. Unfortunately there is no "machine" or "environment" object that could be injected to these classes as mocks that could take care of time-related fixtures.
Thanks,

Zoltán Tóth-Czifra

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7135/
-----------------------------------------------------------

(Updated Oct. 4, 2012, 12:25 p.m.)
Review request for Sqoop.
Changes
-------

Sorry, I'm retarded.
Description
-------

Code review for SQOOP-604, see https://issues.apache.org/jira/browse/SQOOP-604

The solution in short: Using the already existing "checkpoint" feature of the direct (--direct) MySQL exports (the export process is restarted every X bytes written), extending it with a new config value that would simply make the thread sleep for X milliseconds at the checkbpoints. With low enough byte count limit this can be a simple yet powerful throttling mechanism.
Diffs (updated)
-----

  src/java/org/apache/sqoop/mapreduce/MySQLExportMapper.java a4e8b88

Diff: https://reviews.apache.org/r/7135/diff/
Testing
-------

Executing with different settings of sqoop.mysql.export.checkpoint.bytes and sqoop.mysql.export.sleep.ms:

33554432B / 0ms: Transferred 4.7579 MB in 8.7175 seconds (558.8826 KB/sec)
102400B / 500ms: Transferred 4.7579 MB in 35.7794 seconds (136.1698 KB/sec)
51200B / 500ms: Transferred 4.758 MB in 57.8675 seconds (84.1959 KB/sec)
51200B / 250ms: Transferred 4.7579 MB in 35.0293 seconds (139.0854 KB/sec)

I did not add unit tests yet and as it involves calling to Thread.sleep, I find testing this difficult. Unfortunately there is no "machine" or "environment" object that could be injected to these classes as mocks that could take care of time-related fixtures.
Thanks,

Zoltán Tóth-Czifra

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7135/#review12934
-----------------------------------------------------------
My aplogies for replying late on this, had been a lot busy.

I think we will have to do a round more :). I see 5 checkstyle errors, out of which please resolve the 3 (in file MySQLExportMapper.java) which are due to the changes you made in the code. I will raise a JIRA for other two which are not a part of your change.

How to 'checkstyle' steps:
1. run 'ant checkstyle'
2. Now above command may be successful, but it generates an HTML file(checkstyle-errors.html to be exact) in 'build' dir under $SQOOP_HOME. Opening that file gives the details of the checkstyle errrors.

Thanks.

- Abhijeet Gaikwad
On Oct. 4, 2012, 12:25 p.m., Zoltán Tóth-Czifra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7135/
> -----------------------------------------------------------
>
> (Updated Oct. 4, 2012, 12:25 p.m.)
>
>
> Review request for Sqoop.
>
>
> Description
> -------
>
> Code review for SQOOP-604, see https://issues.apache.org/jira/browse/SQOOP-604
>
> The solution in short: Using the already existing "checkpoint" feature of the direct (--direct) MySQL exports (the export process is restarted every X bytes written), extending it with a new config value that would simply make the thread sleep for X milliseconds at the checkbpoints. With low enough byte count limit this can be a simple yet powerful throttling mechanism.
>
>
> Diffs
> -----
>
>   src/java/org/apache/sqoop/mapreduce/MySQLExportMapper.java a4e8b88
>
> Diff: https://reviews.apache.org/r/7135/diff/
>
>
> Testing
> -------
>
> Executing with different settings of sqoop.mysql.export.checkpoint.bytes and sqoop.mysql.export.sleep.ms:
>
> 33554432B / 0ms: Transferred 4.7579 MB in 8.7175 seconds (558.8826 KB/sec)
> 102400B / 500ms: Transferred 4.7579 MB in 35.7794 seconds (136.1698 KB/sec)
> 51200B / 500ms: Transferred 4.758 MB in 57.8675 seconds (84.1959 KB/sec)
> 51200B / 250ms: Transferred 4.7579 MB in 35.0293 seconds (139.0854 KB/sec)
>
> I did not add unit tests yet and as it involves calling to Thread.sleep, I find testing this difficult. Unfortunately there is no "machine" or "environment" object that could be injected to these classes as mocks that could take care of time-related fixtures.
>
>
> Thanks,
>
> Zoltán Tóth-Czifra
>
>

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7135/
-----------------------------------------------------------

(Updated Nov. 2, 2012, 12:32 p.m.)
Review request for Sqoop.
Changes
-------

Sure thing, I'm sorry. Checkstyle passes now with my changes.
Description
-------

Code review for SQOOP-604, see https://issues.apache.org/jira/browse/SQOOP-604

The solution in short: Using the already existing "checkpoint" feature of the direct (--direct) MySQL exports (the export process is restarted every X bytes written), extending it with a new config value that would simply make the thread sleep for X milliseconds at the checkbpoints. With low enough byte count limit this can be a simple yet powerful throttling mechanism.
Diffs (updated)
-----

  src/java/org/apache/sqoop/mapreduce/MySQLExportMapper.java a4e8b88

Diff: https://reviews.apache.org/r/7135/diff/
Testing
-------

Executing with different settings of sqoop.mysql.export.checkpoint.bytes and sqoop.mysql.export.sleep.ms:

33554432B / 0ms: Transferred 4.7579 MB in 8.7175 seconds (558.8826 KB/sec)
102400B / 500ms: Transferred 4.7579 MB in 35.7794 seconds (136.1698 KB/sec)
51200B / 500ms: Transferred 4.758 MB in 57.8675 seconds (84.1959 KB/sec)
51200B / 250ms: Transferred 4.7579 MB in 35.0293 seconds (139.0854 KB/sec)

I did not add unit tests yet and as it involves calling to Thread.sleep, I find testing this difficult. Unfortunately there is no "machine" or "environment" object that could be injected to these classes as mocks that could take care of time-related fixtures.
Thanks,

Zoltán Tóth-Czifra

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7135/#review13075
-----------------------------------------------------------

Ship it!
Looks good :)
ant checkstyle - no errors
ant test - success

- Abhijeet Gaikwad
On Nov. 2, 2012, 12:32 p.m., Zoltán Tóth-Czifra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7135/
> -----------------------------------------------------------
>
> (Updated Nov. 2, 2012, 12:32 p.m.)
>
>
> Review request for Sqoop.
>
>
> Description
> -------
>
> Code review for SQOOP-604, see https://issues.apache.org/jira/browse/SQOOP-604
>
> The solution in short: Using the already existing "checkpoint" feature of the direct (--direct) MySQL exports (the export process is restarted every X bytes written), extending it with a new config value that would simply make the thread sleep for X milliseconds at the checkbpoints. With low enough byte count limit this can be a simple yet powerful throttling mechanism.
>
>
> Diffs
> -----
>
>   src/java/org/apache/sqoop/mapreduce/MySQLExportMapper.java a4e8b88
>
> Diff: https://reviews.apache.org/r/7135/diff/
>
>
> Testing
> -------
>
> Executing with different settings of sqoop.mysql.export.checkpoint.bytes and sqoop.mysql.export.sleep.ms:
>
> 33554432B / 0ms: Transferred 4.7579 MB in 8.7175 seconds (558.8826 KB/sec)
> 102400B / 500ms: Transferred 4.7579 MB in 35.7794 seconds (136.1698 KB/sec)
> 51200B / 500ms: Transferred 4.758 MB in 57.8675 seconds (84.1959 KB/sec)
> 51200B / 250ms: Transferred 4.7579 MB in 35.0293 seconds (139.0854 KB/sec)
>
> I did not add unit tests yet and as it involves calling to Thread.sleep, I find testing this difficult. Unfortunately there is no "machine" or "environment" object that could be injected to these classes as mocks that could take care of time-related fixtures.
>
>
> Thanks,
>
> Zoltán Tóth-Czifra
>
>

> On Nov. 3, 2012, 5:18 a.m., Abhijeet Gaikwad wrote:
> > Looks good :)
> > ant checkstyle - no errors
> > ant test - success

Thank you for your help Abhijeet!
- Zoltán
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7135/#review13075
-----------------------------------------------------------
On Nov. 2, 2012, 12:32 p.m., Zoltán Tóth-Czifra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7135/
> -----------------------------------------------------------
>
> (Updated Nov. 2, 2012, 12:32 p.m.)
>
>
> Review request for Sqoop.
>
>
> Description
> -------
>
> Code review for SQOOP-604, see https://issues.apache.org/jira/browse/SQOOP-604
>
> The solution in short: Using the already existing "checkpoint" feature of the direct (--direct) MySQL exports (the export process is restarted every X bytes written), extending it with a new config value that would simply make the thread sleep for X milliseconds at the checkbpoints. With low enough byte count limit this can be a simple yet powerful throttling mechanism.
>
>
> Diffs
> -----
>
>   src/java/org/apache/sqoop/mapreduce/MySQLExportMapper.java a4e8b88
>
> Diff: https://reviews.apache.org/r/7135/diff/
>
>
> Testing
> -------
>
> Executing with different settings of sqoop.mysql.export.checkpoint.bytes and sqoop.mysql.export.sleep.ms:
>
> 33554432B / 0ms: Transferred 4.7579 MB in 8.7175 seconds (558.8826 KB/sec)
> 102400B / 500ms: Transferred 4.7579 MB in 35.7794 seconds (136.1698 KB/sec)
> 51200B / 500ms: Transferred 4.758 MB in 57.8675 seconds (84.1959 KB/sec)
> 51200B / 250ms: Transferred 4.7579 MB in 35.0293 seconds (139.0854 KB/sec)
>
> I did not add unit tests yet and as it involves calling to Thread.sleep, I find testing this difficult. Unfortunately there is no "machine" or "environment" object that could be injected to these classes as mocks that could take care of time-related fixtures.
>
>
> Thanks,
>
> Zoltán Tóth-Czifra
>
>

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7135/#review12019
-----------------------------------------------------------

src/java/org/apache/sqoop/mapreduce/MySQLExportMapper.java
<https://reviews.apache.org/r/7135/#comment25638>

    What happens when MYSQL_CHECKPOINT_SLEEP_KEY is greater than mapred.task.timeout?
    
    If the job is killed, we need to handle the scenario.
- Abhijeet Gaikwad
On Sept. 27, 2012, 3:47 p.m., Zoltán Tóth-Czifra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7135/
> -----------------------------------------------------------
>
> (Updated Sept. 27, 2012, 3:47 p.m.)
>
>
> Review request for Sqoop.
>
>
> Description
> -------
>
> Code review for SQOOP-604, see https://issues.apache.org/jira/browse/SQOOP-604
>
> The solution in short: Using the already existing "checkpoint" feature of the direct (--direct) MySQL exports (the export process is restarted every X bytes written), extending it with a new config value that would simply make the thread sleep for X milliseconds at the checkbpoints. With low enough byte count limit this can be a simple yet powerful throttling mechanism.
>
>
> Diffs
> -----
>
>   src/java/org/apache/sqoop/mapreduce/MySQLExportMapper.java a4e8b88
>
> Diff: https://reviews.apache.org/r/7135/diff/
>
>
> Testing
> -------
>
> Executing with different settings of sqoop.mysql.export.checkpoint.bytes and sqoop.mysql.export.sleep.ms:
>
> 33554432B / 0ms: Transferred 4.7579 MB in 8.7175 seconds (558.8826 KB/sec)
> 102400B / 500ms: Transferred 4.7579 MB in 35.7794 seconds (136.1698 KB/sec)
> 51200B / 500ms: Transferred 4.758 MB in 57.8675 seconds (84.1959 KB/sec)
> 51200B / 250ms: Transferred 4.7579 MB in 35.0293 seconds (139.0854 KB/sec)
>
> I did not add unit tests yet and as it involves calling to Thread.sleep, I find testing this difficult. Unfortunately there is no "machine" or "environment" object that could be injected to these classes as mocks that could take care of time-related fixtures.
>
>
> Thanks,
>
> Zoltán Tóth-Czifra
>
>

> On Sept. 28, 2012, 9:59 a.m., Abhijeet Gaikwad wrote:
> > src/java/org/apache/sqoop/mapreduce/MySQLExportMapper.java, line 329
> > <https://reviews.apache.org/r/7135/diff/1/?file=155911#file155911line329>
> >
> >     What happens when MYSQL_CHECKPOINT_SLEEP_KEY is greater than mapred.task.timeout?
> >    
> >     If the job is killed, we need to handle the scenario.

That's a good point!

Given that the default value of mapred.task.timeout is 600000 (10m) I consider this very unlikely, the ideal value of the new config key has order of magniture of a few hundred ms. However, in some extreme cases (or when clearly misusing this feature) it is possible that this case needs to be handled.

Do you have any suggestion? For example, limiting sqoop.mysql.export.sleep.ms to a maximum of the value in mapred.task.timeout?
- Zoltán
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7135/#review12019
-----------------------------------------------------------
On Sept. 27, 2012, 3:47 p.m., Zoltán Tóth-Czifra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7135/
> -----------------------------------------------------------
>
> (Updated Sept. 27, 2012, 3:47 p.m.)
>
>
> Review request for Sqoop.
>
>
> Description
> -------
>
> Code review for SQOOP-604, see https://issues.apache.org/jira/browse/SQOOP-604
>
> The solution in short: Using the already existing "checkpoint" feature of the direct (--direct) MySQL exports (the export process is restarted every X bytes written), extending it with a new config value that would simply make the thread sleep for X milliseconds at the checkbpoints. With low enough byte count limit this can be a simple yet powerful throttling mechanism.
>
>
> Diffs
> -----
>
>   src/java/org/apache/sqoop/mapreduce/MySQLExportMapper.java a4e8b88
>
> Diff: https://reviews.apache.org/r/7135/diff/
>
>
> Testing
> -------
>
> Executing with different settings of sqoop.mysql.export.checkpoint.bytes and sqoop.mysql.export.sleep.ms:
>
> 33554432B / 0ms: Transferred 4.7579 MB in 8.7175 seconds (558.8826 KB/sec)
> 102400B / 500ms: Transferred 4.7579 MB in 35.7794 seconds (136.1698 KB/sec)
> 51200B / 500ms: Transferred 4.758 MB in 57.8675 seconds (84.1959 KB/sec)
> 51200B / 250ms: Transferred 4.7579 MB in 35.0293 seconds (139.0854 KB/sec)
>
> I did not add unit tests yet and as it involves calling to Thread.sleep, I find testing this difficult. Unfortunately there is no "machine" or "environment" object that could be injected to these classes as mocks that could take care of time-related fixtures.
>
>
> Thanks,
>
> Zoltán Tóth-Czifra
>
>

> On Sept. 28, 2012, 9:59 a.m., Abhijeet Gaikwad wrote:
> > src/java/org/apache/sqoop/mapreduce/MySQLExportMapper.java, line 329
> > <https://reviews.apache.org/r/7135/diff/1/?file=155911#file155911line329>
> >
> >     What happens when MYSQL_CHECKPOINT_SLEEP_KEY is greater than mapred.task.timeout?
> >    
> >     If the job is killed, we need to handle the scenario.
>
> Zoltán Tóth-Czifra wrote:
>     That's a good point!
>    
>     Given that the default value of mapred.task.timeout is 600000 (10m) I consider this very unlikely, the ideal value of the new config key has order of magniture of a few hundred ms. However, in some extreme cases (or when clearly misusing this feature) it is possible that this case needs to be handled.
>    
>     Do you have any suggestion? For example, limiting sqoop.mysql.export.sleep.ms to a maximum of the value in mapred.task.timeout?

We can go with the solution you suggested.
- Abhijeet
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7135/#review12019
-----------------------------------------------------------
On Sept. 27, 2012, 3:47 p.m., Zoltán Tóth-Czifra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7135/
> -----------------------------------------------------------
>
> (Updated Sept. 27, 2012, 3:47 p.m.)
>
>
> Review request for Sqoop.
>
>
> Description
> -------
>
> Code review for SQOOP-604, see https://issues.apache.org/jira/browse/SQOOP-604
>
> The solution in short: Using the already existing "checkpoint" feature of the direct (--direct) MySQL exports (the export process is restarted every X bytes written), extending it with a new config value that would simply make the thread sleep for X milliseconds at the checkbpoints. With low enough byte count limit this can be a simple yet powerful throttling mechanism.
>
>
> Diffs
> -----
>
>   src/java/org/apache/sqoop/mapreduce/MySQLExportMapper.java a4e8b88
>
> Diff: https://reviews.apache.org/r/7135/diff/
>
>
> Testing
> -------
>
> Executing with different settings of sqoop.mysql.export.checkpoint.bytes and sqoop.mysql.export.sleep.ms:
>
> 33554432B / 0ms: Transferred 4.7579 MB in 8.7175 seconds (558.8826 KB/sec)
> 102400B / 500ms: Transferred 4.7579 MB in 35.7794 seconds (136.1698 KB/sec)
> 51200B / 500ms: Transferred 4.758 MB in 57.8675 seconds (84.1959 KB/sec)
> 51200B / 250ms: Transferred 4.7579 MB in 35.0293 seconds (139.0854 KB/sec)
>
> I did not add unit tests yet and as it involves calling to Thread.sleep, I find testing this difficult. Unfortunately there is no "machine" or "environment" object that could be injected to these classes as mocks that could take care of time-related fixtures.
>
>
> Thanks,
>
> Zoltán Tóth-Czifra
>
>