Skip to content
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

Elasticsearch 1.4.2 #738

Merged
merged 16 commits into from
Jan 21, 2015
Merged

Elasticsearch 1.4.2 #738

merged 16 commits into from
Jan 21, 2015

Conversation

jdeniau
Copy link
Contributor

@jdeniau jdeniau commented Dec 23, 2014

I tried to update to 1.4.2 (based on your 1.4.0 branch).

I now have 8 failing tests:
7 with this message :

Failed asserting that exception of type "Elastica\Exception\ResponseException" matches expected exception "\Elastica\Exception\RuntimeException". Message was: "ClusterBlockException[blocked by: [FORBIDDEN/5/index read-only (api)];]" 

The last one is that Thread.sleep(100); is not authorized anymore now by ES

I do not know how to solve those problems :/

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.87%) when pulling 8d43da9 on jdeniau:elasticsearch-1.4.2 into c8b6848 on ruflin:master.

@ruflin
Copy link
Owner

ruflin commented Dec 23, 2014

I see about 85 tests failing: https://travis-ci.org/ruflin/Elastica/jobs/44939058

One of the problem seems to be the _createIndex function which is used by lots of tests: https://github.com/ruflin/Elastica/blob/master/test/lib/Elastica/Test/Base.php#L23

I assume some parameters with the shards or replicas have changed.

@ruflin
Copy link
Owner

ruflin commented Jan 2, 2015

@jdeniau Did you have any luck yet in fixing the issues?

@jdeniau
Copy link
Contributor Author

jdeniau commented Jan 2, 2015

No sorry I did not look at it this week.
Le 2 janv. 2015 18:27, "Nicolas Ruflin" notifications@github.com a écrit :

@jdeniau https://github.com/jdeniau Did you have any luck yet in fixing
the issues?


Reply to this email directly or view it on GitHub
#738 (comment).

@jdeniau
Copy link
Contributor Author

jdeniau commented Jan 19, 2015

I worked on it today.
The main problem is that the "read_only" instruction seem to block all queries, (see elastic/elasticsearch#9203 ).

I am down to ~15 errors for now, I will commit changes soon.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling 435c724 on jdeniau:elasticsearch-1.4.2 into bca7a12 on ruflin:master.

@ruflin
Copy link
Owner

ruflin commented Jan 19, 2015

@jdeniau That looks really promising. Only 1 error left. Make sure to merge in master as there are also quite a few changes.

I also started to work on it but you are already much farther: https://github.com/ruflin/Elastica/tree/elasticsearch-1.4.0

*/
public function getReadOnly()
{
return $this->get('blocks.write') === 'true'; // ES returns a string for this setting
Copy link
Owner

Choose a reason for hiding this comment

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

This variable sounds so counterintuitive. But it is 100% correct. I also checked the ES docs.

@jdeniau
Copy link
Contributor Author

jdeniau commented Jan 20, 2015

The failing test is really weird.

Actually, it goes well on my local instance of ES.
Moreover the code on master is :

// the document with the random score should have a score > 1, means it is the first result       
$result0 = $results[1]->getData();

But the first document is $results[0], not $results[1] (as said in the variable $result0 too)

@jdeniau jdeniau force-pushed the elasticsearch-1.4.2 branch 2 times, most recently from 4201e39 to 80632d3 Compare January 20, 2015 07:59
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling 6a72343 on jdeniau:elasticsearch-1.4.2 into bca7a12 on ruflin:master.

@jdeniau jdeniau force-pushed the elasticsearch-1.4.2 branch 3 times, most recently from c186d85 to 867ef2a Compare January 20, 2015 09:02
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling 056107e on jdeniau:elasticsearch-1.4.2 into bca7a12 on ruflin:master.

@@ -68,31 +68,33 @@ public function get($setting = '')
$requestData = $this->request()->getData();
$data = reset($requestData);

if (empty($data['settings']) || empty($data['settings']['index'])) {
return []; // index not found
Copy link
Owner

Choose a reason for hiding this comment

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

Should we actually throw a not found exception here?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling 867ef2a on jdeniau:elasticsearch-1.4.2 into bca7a12 on ruflin:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling 867ef2a on jdeniau:elasticsearch-1.4.2 into bca7a12 on ruflin:master.

@jdeniau
Copy link
Contributor Author

jdeniau commented Jan 20, 2015

Sorry to push a lot but I have some differences between travis and my localhost instance :(

I took the responsability of removing the 5.3 support. I only did that to allow array short syntax, so it is easily revertable, but as the 5.3 version is not officially supported by the PHP developers, I think it is a good think to remove it.

But it's up to you, if you want to revert, I'll do it.

@ruflin
Copy link
Owner

ruflin commented Jan 20, 2015

@jdeniau No problem. The main priority is getting 1.4.2 running. To make it then compatible again with 5.3 if necessary is an other thing. Lets not focus on this right now.

@ruflin
Copy link
Owner

ruflin commented Jan 20, 2015

@jdeniau All green. Let me know when I can merge.

@jdeniau
Copy link
Contributor Author

jdeniau commented Jan 20, 2015

OK for me when the next build is OK.

@jdeniau
Copy link
Contributor Author

jdeniau commented Jan 20, 2015

It's OK for me ! :)

env:
global:
Copy link
Contributor

Choose a reason for hiding this comment

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

This block(9-16) was deleted by me in #742.
Packages versions should be specified in ansible/es-playbook.yml now.
Actually, travis built your pull request with ES 1.3.4, as i can see from logs :)

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for checking ...

@ruflin
Copy link
Owner

ruflin commented Jan 20, 2015

@jdeniau Can you please have a look at this?

@jdeniau
Copy link
Contributor Author

jdeniau commented Jan 21, 2015

Finally 😎
I am not proud of this commit jdeniau@07b56f6 but it is the same kind of problem we have with the readonly.

Once merged, you can merge #735 (I updated it)

@@ -12,10 +12,10 @@
}
],
"require": {
"php": ">=5.3.3"
"php": ">=5.3.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

With arrays new syntax, ">=5.4.0"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that but got a weird bug with composer in the tests installation process:
https://travis-ci.org/ruflin/Elastica/builds/47757241

Copy link
Contributor

Choose a reason for hiding this comment

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

I've found a problem.

  1. ubuntu 12.04 default php version is 5.3
  2. ansible uses sudo internally for all steps
  3. when you use phpenv as travis does, php and sudo php is different versions of php
  4. sudo composer can't satisfy ">=5.4" requirement, becase uses system php, which 5.3
  5. ansible wipe out all stderr from composer so it fails silently =/

Moreover, vagrant box is broken again, as there only one php version - 5.3

I'll fix this issue.

Copy link
Owner

Choose a reason for hiding this comment

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

Thx. Looking forward to it.

ruflin added a commit that referenced this pull request Jan 21, 2015
@ruflin ruflin merged commit 9d284d5 into ruflin:master Jan 21, 2015
@ruflin
Copy link
Owner

ruflin commented Jan 21, 2015

@jdeniau I merged it in :-) That was a big pice of work. Thanks a lot.

Will you keep an eye on elastic/elasticsearch#9203 ? I assume as soon as it is merged in and release, it will break again the implementation.

@ruflin
Copy link
Owner

ruflin commented Jan 21, 2015

@jdeniau BTW: I think we forgot to update the README: https://github.com/ruflin/Elastica/blob/master/README.markdown

@jdeniau jdeniau deleted the elasticsearch-1.4.2 branch January 22, 2015 07:21
@jdeniau
Copy link
Contributor Author

jdeniau commented Jan 22, 2015

I created another PR #756 for this.

Thanks for the merge !

For the metadata bug on ES itself, it will be part of a new ElasticSearch version. I'll watch about it.

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