arangodb / arangodb-php

PHP ODM for ArangoDB
https://www.arangodb.com
Apache License 2.0
183 stars 43 forks source link

GraphHandler.saveVertex() not 'batchable' using php 7.0?! #190

Closed tomterl closed 7 years ago

tomterl commented 8 years ago

Platform: Linux Ubuntu 16.04 x86_64, php 7.0.4, arangodb-php branch 2.8

If I have an active batch, using saveVertex fails because the there called getVertexCollections does not get the vertex-collection names as response, but

    [_id:protected] => 0/0
    [_key:protected] => 
    [_rev:protected] => 0
    [_values:protected] => Array
        (
            [error] => 
            [id] => 0
            [hasMore] => 1
            [result] => Array
                (
                    [0] => Array
                        (
                        )

                )

            [documents] => Array
                (
                    [0] => Array
                        (
                        )

                )

            [name] => integer
            [type] => base
        )

    [_changed:protected] => 1
    [_isNew:protected] => 
    [_doValidate:protected] => 
    [_hidden:protected] => Array 

This did not happen using php-5.3. Other reading accesses to the connection in batch mode produce the same result -- for now I store the vertices as documents via DocumentHandler.save(), which works just fine.

olegsv commented 7 years ago

Was there any progress with that?

frankmayer commented 7 years ago

Hi, I took a quick look at the code. If I am not mistaken, the saveVertex() method should not have been able to work ever in batch mode, in any PHP version.

That is due to the way the saveVertex() method and the batch mode are implemented. The response you posted in your opening comment is actually a pseudo-response generated from the driver itself, in order for the "request" to "end" and the driver to be able to continue processing the next commands. The batch mode in the driver was implemented after the complete skeleton of the methods was already there and needed to work behind the scenes, in order not to break backwards compatibility.

Having said that I think that Batch mode is a bit difficult to do, as it would probably require async and callbacks in order to function properly - or a totally different driver setup.

I fear that the suggested workaround will be the way to go in this case - at least for the moment.

Any thoughts?

tomterl commented 7 years ago

I'm fine using the workaround. Thanks for looking into this.

frankmayer commented 7 years ago

So, I had some thoughts on batch mode in general and recently pushed some changes onto the devel branch, which should deal with this exact issue and also be more performant due to a "Cache" implementation for the GraphHandler. Basically, it should deal with saveVertex and saveEdge in batch mode just fine, now. I'd like to hear your comments/suggestions, if you get around to play with it.

tomterl commented 7 years ago

Sounds good, I'll check it out as soon as the migration to 3.0 is done. I hope it's not that long a wait...

tomterl commented 7 years ago

Are your changes in the 3.1-branch? I updated our version to that branch and reverted my changes back to use saveVertex() and it works; unfortunately I can't provide you with performance comparison data, as my setup tends to produce widely differing running times for the same job, due to me pushing php-7s array implementation around a bit.

If I can test code-paths for you, just let me know, I can setup a test-case with a smaller scale producing comparable results.

frankmayer commented 7 years ago

Hi @tomterl, glad, it worked for you. Yes, those and a lot of other changes are in the 3.1 branch (also 3.0, but the latest commits had no tag 3.0.x, as 3.1.0 was due).

Yes, If it is no trouble for you, it would be great if we had a real-world comparison of the performance (Also thinking of the new GraphHandler Cache, using GraphHandler->setCacheEnabled(true)).