You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@predictionio.apache.org by marevol <gi...@git.apache.org> on 2017/02/26 09:38:03 UTC
[GitHub] incubator-predictionio pull request #352: [PIO-49] Add support for Elasticse...
GitHub user marevol opened a pull request:
https://github.com/apache/incubator-predictionio/pull/352
[PIO-49] Add support for Elasticsearch 5 and move storage implementation
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/marevol/incubator-predictionio move-storages
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-predictionio/pull/352.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #352
----
commit 3227fc7c270729df5e45fd0693333f93201851dd
Author: Shinsuke Sugaya <sh...@yahoo.co.jp>
Date: 2017-02-26T09:31:50Z
move storage implementation to storage directories
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-predictionio issue #352: [PIO-49] Add support for Elasticsearch 5 ...
Posted by marevol <gi...@git.apache.org>.
Github user marevol commented on the issue:
https://github.com/apache/incubator-predictionio/pull/352
For integration tests of ES5, 3 tests are failed on [move-storages-es5]...(https://github.com/marevol/incubator-predictionio/tree/move-storages-es5)
I'll check them today.
```
======================================================================
ERROR [0.555s]: runTest (pio_tests.scenarios.quickstart_test.QuickStartTest)
----------------------------------------------------------------------
Traceback (most recent call last):
File "//PredictionIO/tests/pio_tests/scenarios/quickstart_test.py", line 67, in setUp
.format(self.training_data_path))
File "/PredictionIO/tests/pio_tests/utils.py", line 33, in srun
stderr=globals.std_err(), check=True)
File "/usr/lib/python3.5/subprocess.py", line 708, in run
output=stdout, stderr=stderr)
subprocess.CalledProcessError: Command 'curl https://raw.githubusercontent.com/apache/spark/master/data/mllib/sample_movielens_data.txt --create-dirs -o /PredictionIO/tests/pio_tests/data/quickstart_test/training_data.txt' returned non-zero exit status 6
======================================================================
ERROR [510.444s]: runTest (pio_tests.scenarios.eventserver_test.EventserverTest)
----------------------------------------------------------------------
Traceback (most recent call last):
File "//PredictionIO/tests/pio_tests/scenarios/eventserver_test.py", line 96, in runTest
self.load_events("signup_events_51.json"))
File "/PredictionIO/tests/pio_tests/utils.py", line 316, in import_events_batch
return import_events_batch(events, self.test_context, self.id)
File "/PredictionIO/tests/pio_tests/utils.py", line 157, in import_events_batch
'--channel {}'.format(channel) if channel else ''))
File "/PredictionIO/tests/pio_tests/utils.py", line 33, in srun
stderr=globals.std_err(), check=True)
File "/usr/lib/python3.5/subprocess.py", line 708, in run
output=stdout, stderr=stderr)
subprocess.CalledProcessError: Command 'pio import --appid 1 --input /PredictionIO/tests/pio_tests/data/events.json.tmp ' returned non-zero exit status 1
======================================================================
ERROR [116.174s]: runTest (pio_tests.scenarios.basic_app_usecases.BasicAppUsecases)
----------------------------------------------------------------------
Traceback (most recent call last):
File "//PredictionIO/tests/pio_tests/scenarios/basic_app_usecases.py", line 80, in runTest
self.check_data()
File "//PredictionIO/tests/pio_tests/scenarios/basic_app_usecases.py", line 110, in check_data
self.assertEqual(len(buy_events) + len(rate_events), len(r.json()))
AssertionError: 40 != 39
----------------------------------------------------------------------
Ran 3 tests in 627.175s
```
> use ES5 as default
This PR puts ES1 jar file to lib/spark/ and ES5 jar to extra/ in PredictionIO-*.tar.gz.
It meant that it's replaced with ES5 jar in lib/spark/.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-predictionio pull request #352: [PIO-49] Add support for Elasticse...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-predictionio/pull/352
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-predictionio issue #352: [PIO-49] Add support for Elasticsearch 5 ...
Posted by dszeto <gi...@git.apache.org>.
Github user dszeto commented on the issue:
https://github.com/apache/incubator-predictionio/pull/352
@marevol Really appreciate this great piece of work! I am planning to make a few minor changes after merging this:
* Make Elasticsearch 1 assembly default (by putting that in `lib/spark` instead of using the ES5 assembly. This is to make sure we allow enough time for deprecation.
* Adjust integration tests accordingly.
* Add integration tests for both ES1 and ES5.
* Add a switch in `make-distribution.sh --use-es5` for building a distribution as is this pull request.
Please let me know how you think. The goal is to allow people to upgrade to 0.11 without changing configuration files. We will announce loudly that we will deprecate ES1 and make it non-default in 0.12.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-predictionio issue #352: [PIO-49] Add support for Elasticsearch 5 ...
Posted by dszeto <gi...@git.apache.org>.
Github user dszeto commented on the issue:
https://github.com/apache/incubator-predictionio/pull/352
Hi @marevol , I have finished working on PIO-53, which allows unit and integration tests be updated per commit and removed reliance on a cached Docker image on Docker Hub. You might want to check that out while I review the rest of the code. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-predictionio issue #352: [PIO-49] Add support for Elasticsearch 5 ...
Posted by dszeto <gi...@git.apache.org>.
Github user dszeto commented on the issue:
https://github.com/apache/incubator-predictionio/pull/352
So basically this will build PIO with the same behavior as before, and pack the ES5 support as an extra JAR during `make-distribution.sh` in `lib/extra`. In order to use ES5, one would remove the ES1 JAR in `lib/spark` and move the ES5 JAR to `lib/spark`. There would be minimal change needed to `conf/pio-env.sh` since their keys are shared. Correct @marevol? This essentially does not require a separate build while allowing flexibility to use a different backend support. @pferrel any more comments? I'll do a little more testing and merge if this sounds good to you.
Thanks for the hard work @haginot and @marevol !
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-predictionio issue #352: [PIO-49] Add support for Elasticsearch 5 ...
Posted by marevol <gi...@git.apache.org>.
Github user marevol commented on the issue:
https://github.com/apache/incubator-predictionio/pull/352
Almost all failures are fixed.
For remaining problem, integration tests use PDT time, but elasticsearch uses UTC time.
Therefore, the following assertion is failed in tests/pio_tests/scenarios/eventserver_test.py.
```
self.assertEqual('2014-11-05T09:39:45.618-08:00', r.json()[0]['eventTime'])
```
I think it's better to use UTC time.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-predictionio issue #352: [PIO-49] Add support for Elasticsearch 5 ...
Posted by marevol <gi...@git.apache.org>.
Github user marevol commented on the issue:
https://github.com/apache/incubator-predictionio/pull/352
I'll fix it today...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-predictionio issue #352: [PIO-49] Add support for Elasticsearch 5 ...
Posted by marevol <gi...@git.apache.org>.
Github user marevol commented on the issue:
https://github.com/apache/incubator-predictionio/pull/352
Fixed!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-predictionio issue #352: [PIO-49] Add support for Elasticsearch 5 ...
Posted by dszeto <gi...@git.apache.org>.
Github user dszeto commented on the issue:
https://github.com/apache/incubator-predictionio/pull/352
Adding @haginot to this thread.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-predictionio issue #352: [PIO-49] Add support for Elasticsearch 5 ...
Posted by dszeto <gi...@git.apache.org>.
Github user dszeto commented on the issue:
https://github.com/apache/incubator-predictionio/pull/352
Hey @marevol are you working on fixing the integration tests? I can help if you are not already doing so.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-predictionio issue #352: [PIO-49] Add support for Elasticsearch 5 ...
Posted by marevol <gi...@git.apache.org>.
Github user marevol commented on the issue:
https://github.com/apache/incubator-predictionio/pull/352
Correct.
I'm working on integration tests for ES5. To use ES5 as default, should I create a new PR?
For UR, @haginot and co-workers work on ES5 support. They will create PR to UR repository, maybe this week...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-predictionio issue #352: [PIO-49] Add support for Elasticsearch 5 ...
Posted by marevol <gi...@git.apache.org>.
Github user marevol commented on the issue:
https://github.com/apache/incubator-predictionio/pull/352
Fixed problems for ES5 support.
Integration tests will be passed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-predictionio issue #352: [PIO-49] Add support for Elasticsearch 5 ...
Posted by dszeto <gi...@git.apache.org>.
Github user dszeto commented on the issue:
https://github.com/apache/incubator-predictionio/pull/352
@marevol Super cool. How's integration tests? And what do you mean by "use ES5 as default"?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---