aerospike-community / aerospike-client-php

Aerospike client for PHP7
Apache License 2.0
29 stars 28 forks source link

Aerospike Object - Allow disabling SHM by passing FALSE as shm value. #43

Closed vityank closed 4 years ago

dwelch-spike commented 4 years ago

I don't think this change is necessary since shared memory is disabled by default. See here

vityank commented 4 years ago

We have it enabled. However in code there we need to connect to multiple clusters, we disable it.

ср, 18 дек. 2019 г., 2:15 AM dwelch-spike notifications@github.com:

I don't think this change is necessary since shared memory is disabled by default. See here https://www.aerospike.com/docs/client/c/usage/shm.html

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/aerospike/aerospike-client-php/pull/43?email_source=notifications&email_token=AEMMG76WFNUVA2FILBGTYL3QZFTQZA5CNFSM4JIXSPXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHEMY5I#issuecomment-566807669, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEMMG73IRAJVGHZILFLKZW3QZFTQZANCNFSM4JIXSPXA .

dwelch-spike commented 4 years ago

If you have a unique shm_key for each cluster, you can connect multiple clients to the those clusters with shared memory enabled. As long as the clients are configured to use the appropriate shm_keys they will not interfere with each other.

vityank commented 4 years ago

In such a case We'll need to manage several SHM addresses, which is not safe. Taking the C library history, into consideration where SHM address was changed due to changes in internal binary format makes it even more dangerous. The point is simplicity w/o config files diversifications along servers parks. If this patch is so 'disliked', then there is no much I can do to convince you. At worst case it will remain local extra patch.

rbotzer commented 4 years ago

From the client config documentation:

shm_key

Explicitly set the shm key for this client. If use_shared_connection is not set, or set to False, the user must provide a value for this field in order for shared memory to work correctly. If , and only if, use_shared_connection is set to True, the key will be implicitly evaluated per unique hostname, and can be inspected with shm_key() . It is still possible to specify a key when using use_shared_connection = True.

I'm not sure where the disconnect is - you already have full control to configure the behavior you want in the application. And if you're modifying the application for this already, I don't see how it's more or less dangerous than other options.

We're trying to avoid having multiple paths to do the same thing. Like you mentioned, you can fork the client.

vityank commented 4 years ago

What a client for Python has to do with one for PHP? PHP client relies on C client library and uses default shared memory address. The limitation of PHP client is that you can only disable SHM globally in configuration of the extension, and not in runtime in object. This path allows to disable it.

"If , and only if, use_shared_connection is set to True, the key will be implicitly evaluated per unique hostname, and can be inspected with shm_key() ." Nice for Python. No such logic in PHP extension...

rbotzer commented 4 years ago

@vityank I guess I shouldn't have been looking into it at a weird hour while jetlagged. I worked on the Python and PHP clients in the past, so got some wires crossed.

Still need to understand where the 'safety' issue is. Your application has full control over whether SHM is used or not. You already have aerospike.shm.use = false (and it's the default!) so being able to also set aerospike.shm = false to achieve the same thing gives you no more functionality, and it's odd to have two ways that are so similar to get the same result. (https://www.aerospike.com/apidocs/php/classes/Aerospike.html)

aerospike.shm.use is the top thing that controls if SHM is use or not. Changing the aerospike.shm.key doesn't modify this behavior, so it should be adequate.

dwelch-spike commented 4 years ago

Even if "use_shm" is enabled in the php.ini, it will still be disabled by default in the clients unless a "shm" array is passed in the config. See this test for an example. You can disable shm on a client by client basis by omitting the shm array in the config.

vityank commented 4 years ago

Interesting, it didn't worked for me then I developed the Aspike connector... The false patch worked. The bug i tried to avoid, which was tested by our own autetests was as following: 1) $x=connect to cluster A 2) $y=connect to unreachable cluster B

W/o patch, even empty shm $y will end as connection to cluster B.

сб, 21 дек. 2019 г., 1:23 AM dwelch-spike notifications@github.com:

Even if "use_shm" is enabled in the php.ini, it will still be disabled by default in the clients unless a "shm" array is passed in the config. See this test https://github.com/aerospike/aerospike-client-php/blob/5d61b2d76bdfdbf09daac160e27a1111dc788657/src/tests/Connection.inc#L437 for an example. You can disable shm on a client by client basis by omitting the shm array in the config.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/aerospike/aerospike-client-php/pull/43?email_source=notifications&email_token=AEMMG74MBLOPAR3WZNSNZSDQZVHVLA5CNFSM4JIXSPXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHON73A#issuecomment-568123372, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEMMG77IJ4PNLZACRX353P3QZVHVLANCNFSM4JIXSPXA .

vityank commented 4 years ago

Mistyped. $y ended as connection to cluster A, and this is incorrect behaviour.

сб, 21 дек. 2019 г., 1:53 AM Виктор К. vityan888@gmail.com:

Interesting, it didn't worked for me then I developed the Aspike connector... The false patch worked. The bug i tried to avoid, which was tested by our own autetests was as following: 1) $x=connect to cluster A 2) $y=connect to unreachable cluster B

W/o patch, even empty shm $y will end as connection to cluster B.

сб, 21 дек. 2019 г., 1:23 AM dwelch-spike notifications@github.com:

Even if "use_shm" is enabled in the php.ini, it will still be disabled by default in the clients unless a "shm" array is passed in the config. See this test https://github.com/aerospike/aerospike-client-php/blob/5d61b2d76bdfdbf09daac160e27a1111dc788657/src/tests/Connection.inc#L437 for an example. You can disable shm on a client by client basis by omitting the shm array in the config.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/aerospike/aerospike-client-php/pull/43?email_source=notifications&email_token=AEMMG74MBLOPAR3WZNSNZSDQZVHVLA5CNFSM4JIXSPXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHON73A#issuecomment-568123372, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEMMG77IJ4PNLZACRX353P3QZVHVLANCNFSM4JIXSPXA .

vityank commented 4 years ago

We have shm.use=true in our config, as most of the time we need it. Nevermind.

сб, 21 дек. 2019 г., 12:51 AM Ronen Botzer notifications@github.com:

@vityank https://github.com/vityank I guess I shouldn't have been looking into it at a weird hour while jetlagged. I worked ont he Python and PHP clients in the past, so got some wires crossed.

Still need to understand where the 'safety' issue is. Your application has full control over whether SHM is used or not. You already have aerospike.shm.use = false (and it's the default!) so being able to also set aerospike.shm = false to achieve the same thing gives you no more functionality, and it's odd to have two ways that are so similar to get the same result.

aerospike.shm.use is the top thing that controls if SHM is use or not. Changing the aerospike.shm.key doesn't modify this behavior, so it should be adequate.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/aerospike/aerospike-client-php/pull/43?email_source=notifications&email_token=AEMMG724NSRD7IEJ2YKH3JLQZVD7HA5CNFSM4JIXSPXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHOML3Q#issuecomment-568116718, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEMMG77DSJGX2AG5CQLJET3QZVD7HANCNFSM4JIXSPXA .