Skip to content

Conversation

@jerryshao
Copy link
Contributor

Because of lacking of BlockAllocationEvent in WAL recovery, the dangled event will mix into the new batch, which will lead to the wrong result. Details can be seen in SPARK-5233.

@SparkQA
Copy link

SparkQA commented Jan 14, 2015

Test build #25514 has started for PR 4032 at commit 4c82be8.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Jan 14, 2015

Test build #25514 has finished for PR 4032 at commit 4c82be8.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25514/
Test PASSed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you improve the unit test to actually verify the behavior. The unit test line you removed can be modified to verify that calling batchTime = lastAllocatedBatchTime is a no-op and does no allocation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I will improve this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update to this comment. See other comment about removing this check altogether. Please update unit test to verify this new behavior.

@tdas
Copy link
Contributor

tdas commented Jan 22, 2015

I understand this patch now. Please update it based on my comments, and test it in your harness to make sure that it addresses the exception problem. If this can be done by US time morning, it can make it to 1.2.1

@jerryshao
Copy link
Contributor Author

OK, will do, thanks a lot for your comments.

@SparkQA
Copy link

SparkQA commented Jan 22, 2015

Test build #25964 has started for PR 4032 at commit a237c75.

  • This patch merges cleanly.

@jerryshao
Copy link
Contributor Author

Hey @tdas , I've updated the code and rebased the branch according to your comments, also with several rounds of test in my local set, the previous exception I reported is gone :).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grammar fix
"..some blocks that were added but not allocated, are dangling in the queue after recovering. We have to allocate thoe blocks to the next batch, which is the batch they were supposed to go to."

@tdas
Copy link
Contributor

tdas commented Jan 22, 2015

looks almost good. some minor comments.

@SparkQA
Copy link

SparkQA commented Jan 22, 2015

Test build #25964 has finished for PR 4032 at commit a237c75.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25964/
Test PASSed.

@SparkQA
Copy link

SparkQA commented Jan 22, 2015

Test build #25966 has started for PR 4032 at commit f0b0c0b.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Jan 22, 2015

Test build #25966 has finished for PR 4032 at commit f0b0c0b.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25966/
Test PASSed.

@tdas
Copy link
Contributor

tdas commented Jan 23, 2015

LGTM. Merging this. Thanks!!!

@tdas
Copy link
Contributor

tdas commented Jan 23, 2015

LGTM. Merging this. Thanks!

@asfgit asfgit closed this in 3c3fa63 Jan 23, 2015
asfgit pushed a commit that referenced this pull request Jan 23, 2015
Because of lacking of `BlockAllocationEvent` in WAL recovery, the dangled event will mix into the new batch, which will lead to the wrong result. Details can be seen in [SPARK-5233](https://issues.apache.org/jira/browse/SPARK-5233).

Author: jerryshao <[email protected]>

Closes #4032 from jerryshao/SPARK-5233 and squashes the following commits:

f0b0c0b [jerryshao] Further address the comments
a237c75 [jerryshao] Address the comments
e356258 [jerryshao] Fix bug in unit test
558bdc3 [jerryshao] Correctly replay the WAL log when recovering from failure

(cherry picked from commit 3c3fa63)
Signed-off-by: Tathagata Das <[email protected]>
@jerryshao
Copy link
Contributor Author

Thanks for your reviewing :)

scwf pushed a commit to scwf/spark that referenced this pull request Jan 25, 2015
commit ea74365
Author: Xiangrui Meng <[email protected]>
Date:   Thu Jan 22 22:09:13 2015 -0800

    [SPARK-3541][MLLIB] New ALS implementation with improved storage

    This PR adds a new ALS implementation to `spark.ml` using the pipeline API, which should be able to scale to billions of ratings. Compared with the ALS under `spark.mllib`, the new implementation

    1. uses the same algorithm,
    2. uses float type for ratings,
    3. uses primitive arrays to avoid GC,
    4. sorts and compresses ratings on each block so that we can solve least squares subproblems one by one using only one normal equation instance.

    The following figure shows performance comparison on copies of the Amazon Reviews dataset using a 16-node (m3.2xlarge) EC2 cluster (the same setup as in http://databricks.com/blog/2014/07/23/scalable-collaborative-filtering-with-spark-mllib.html):
    ![als-wip](https://cloud.githubusercontent.com/assets/829644/5659447/4c4ff8e0-96c7-11e4-87a9-73c1c63d07f3.png)

    I keep the `spark.mllib`'s ALS untouched for easy comparison. If the new implementation works well, I'm going to match the features of the ALS under `spark.mllib` and then make it a wrapper of the new implementation, in a separate PR.

    TODO:
    - [X] Add unit tests for implicit preferences.

    Author: Xiangrui Meng <[email protected]>

    Closes apache#3720 from mengxr/SPARK-3541 and squashes the following commits:

    1b9e852 [Xiangrui Meng] fix compile
    5129be9 [Xiangrui Meng] Merge remote-tracking branch 'apache/master' into SPARK-3541
    dd0d0e8 [Xiangrui Meng] simplify test code
    c627de3 [Xiangrui Meng] add tests for implicit feedback
    b84f41c [Xiangrui Meng] address comments
    a76da7b [Xiangrui Meng] update ALS tests
    2a8deb3 [Xiangrui Meng] add some ALS tests
    857e876 [Xiangrui Meng] add tests for rating block and encoded block
    d3c1ac4 [Xiangrui Meng] rename some classes for better code readability add more doc and comments
    213d163 [Xiangrui Meng] org imports
    771baf3 [Xiangrui Meng] chol doc update
    ca9ad9d [Xiangrui Meng] add unit tests for chol
    b4fd17c [Xiangrui Meng] add unit tests for NormalEquation
    d0f99d3 [Xiangrui Meng] add tests for LocalIndexEncoder
    80b8e61 [Xiangrui Meng] fix imports
    4937fd4 [Xiangrui Meng] update ALS example
    56c253c [Xiangrui Meng] rename product to item
    bce8692 [Xiangrui Meng] doc for parameters and project the output columns
    3f2d81a [Xiangrui Meng] add doc
    1efaecf [Xiangrui Meng] add example code
    8ae86b5 [Xiangrui Meng] add a working copy of the new ALS implementation

commit e0f7fb7
Author: jerryshao <[email protected]>
Date:   Thu Jan 22 22:04:21 2015 -0800

    [SPARK-5315][Streaming] Fix reduceByWindow Java API not work bug

    `reduceByWindow` for Java API is actually not Java compatible, change to make it Java compatible.

    Current solution is to deprecate the old one and add a new API, but since old API actually is not correct, so is keeping the old one meaningful? just to keep the binary compatible? Also even adding new API still need to add to Mima exclusion, I'm not sure to change the API, or deprecate the old API and add a new one, which is the best solution?

    Author: jerryshao <[email protected]>

    Closes apache#4104 from jerryshao/SPARK-5315 and squashes the following commits:

    5bc8987 [jerryshao] Address the comment
    c7aa1b4 [jerryshao] Deprecate the old one to keep binary compatible
    8e9dc67 [jerryshao] Fix JavaDStream reduceByWindow signature error

commit 3c3fa63
Author: jerryshao <[email protected]>
Date:   Thu Jan 22 21:58:53 2015 -0800

    [SPARK-5233][Streaming] Fix error replaying of WAL introduced bug

    Because of lacking of `BlockAllocationEvent` in WAL recovery, the dangled event will mix into the new batch, which will lead to the wrong result. Details can be seen in [SPARK-5233](https://issues.apache.org/jira/browse/SPARK-5233).

    Author: jerryshao <[email protected]>

    Closes apache#4032 from jerryshao/SPARK-5233 and squashes the following commits:

    f0b0c0b [jerryshao] Further address the comments
    a237c75 [jerryshao] Address the comments
    e356258 [jerryshao] Fix bug in unit test
    558bdc3 [jerryshao] Correctly replay the WAL log when recovering from failure
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants