- Feb 04, 2020
-
-
Alexander V. Tikhonov authored
We're going to use S3 compatible storage for Deb and RPM repositories instead of packagecloud.io service. The main reason is that packagecloud.io provides a limited amount of storage, which is not enough for keeping all packages (w/o regular pruning of old versions). Note: At the moment packages are still pushed to packagecloud.io from Travis-CI. Disabling this is out of scope of this patch. This patch implements saving of packages on an S3 compatible storage and regeneration of a repository metadata. The layout is a bit different from one we have on packagecloud.io. packagecloud.io: | - 1.10 | - 2.1 | - 2.2 | - ... S3 compatible storage: | - live | - 1.10 | - 2.1 | - 2.2 | - ... | - release | - 1.10 | - 2.1 | - 2.2 | - ... Both 'live' and 'release' repositories track release branches (named as <major>.<minor>) and master branch. The difference is that 'live' is updated on every push, but 'release' is only for tagged versions (<major>.<minor>.<patch>.0). Packages are also built on '*-full-ci' branches, but only for testing purposes: they don't pushed anywhere. The core logic is in the tools/update_repo.sh script, which implements the following flow: - create metadata for new packages - fetch relevant metadata from the S3 storage - push new packages to the S3 storage - merge and push the updated metadata to the S3 storage The script uses 'createrepo' for RPM repositories and 'reprepro' for Deb repositories. Closes #3380 (cherry picked from commit 05d3ed4b)
-
- Jan 29, 2020
-
-
Kirill Yukhin authored
Revert "Free all slabs on region reset" commit. Closes #4736
-
- Nov 14, 2019
-
-
Alexander Turenko authored
Before commit 03f85d4c ('app: fix boolean handling in argparse module') the module does not expect a value after a 'boolean' argument. However there was the problem: a 'boolean' argument can be passed only at end of an argument list, otherwise it wrongly consumes a next argument and gives a confusing error message. The mentioned commit fixes this behaviour in the following way: it still allows to pass a 'boolean' argument at end of the list w/o a value, but requires a value ('true', 'false', '1', '0') if a 'boolean' argument is not at the end to be provided using {'--foo=true'} or {'--foo', 'true'} syntax. Here this behaviour is changed: a 'boolean' argument does not assume an explicitly passed value despite its position in an argument list. If a 'boolean' argument appears in the list, then argparse.parse() returns `true` for its value (a list of `true` values in case of 'boolean+' argument), otherwise it will not be added to the result. This change also makes the behaviour of long (--foo) and short (-f) 'boolean' options consistent. The motivation of the change is simple: it is easier and more natural to type, say, `tarantoolctl cat --show-system 00000000000000000000.snap` then `tarantoolctl cat --show-system true 00000000000000000000.snap`. This commit adds several new test cases, but it does not mean that we guarantee that the module behaviour will not be changed around some corner cases, say, handling of 'boolean+' arguments. This is internal module. Follows up #4076. Reviewed-by:
Vladislav Shpilevoy <v.shpilevoy@tarantool.org> (cherry picked from commit e47f2c91)
-
- Nov 12, 2019
-
-
Vladislav Shpilevoy authored
The admin user has universal privileges before bootstrap or recovery are done. That allows to, for example, bootstrap from a remote master, because to do that the admin should be able to insert into system spaces, such as _priv. But after the patch on online credentials update was implemented (#2763, 48d00b0e) the admin could loose its universal access if, for example, a role was granted to him before universal access was recovered. That happened by two reasons: - Any change in access rights, even in granted roles, led to rebuild of universal access; - Any change in access rights updated the universal access in all existing sessions, thanks to #2763. What happened: two tarantools were started. One of them master, granted 'replication' role to admin. Second node, slave, tried to bootstrap from the master. The slave created an admin session and started loading data. After it loaded 'grant replication role to admin' command, this nullified admin universal access everywhere, including this session. Next rows could not be applied. Closes #4606 (cherry picked from commit 95237ac8)
-
- Nov 11, 2019
-
-
Alexander V. Tikhonov authored
After the issue #4537 fixed for the data segment size limit, the temporary blocked tests because of it unblocked. Part of #4271 (cherry picked from commit e6866550)
-
- Nov 08, 2019
-
-
Alexander V. Tikhonov authored
Added build + test jobs in GitLab-CI and build + test + deploy jobs on Travis-CI for CentOS 8. Updated testing dependencies in the RPM spec to follow the new Python 2 package naming scheme that was introduced in CentOS 8: it uses 'python2-' prefix rather then 'python-'. CentOS 8 does not provide python2-gevent and python2-greenlet packages, so they were pushed to https://packagecloud.io/packpack/backports repository. This repository is enabled in our build image (packpack/packpack:el-8) by default. Those dependencies are build-time, so nothing was changed for a user. The source RPM packages were gathered from https://cbs.centos.org . Disabled app-tap/pwd.test.lua on CentOS 8 due to systemd-nss issue, which was not worked around properly. Filed #4592 to resolved it in the future. Eliminated libunwind runtime dependency (and libunwind-devel build dependency) on CentOS 8, because the base system does not provide it. fiber.info() backtraces and printing of a backtrace after a crash will not be available on this system. Hopefully we'll fix it in the future, filed #4611 on this. Closes #4543 Reviewed-by:
Alexander Turenko <alexander.turenko@tarantool.org> Reviewed-by:
Igor Munkin <imun@tarantool.org> (cherry picked from commit e3d9d8c9)
-
Alexander Turenko authored
After ea5929db ('build: fix OpenSSL linking problems on FreeBSD') we set CFLAGS explicitly (possibly to an empty value) when invoking a configure script for curl. When this parameter is set the script does not use a value of environment variable CFLAGS. Before this commit LDFLAGS environment variable can affect build of curl submodule. This can lead to a problem when a user or a tool set CFLAGS and LDFLAGS both and some linker flag assumes that some compilation flag is present. Here we set empty LDFLAGS explicitly to avoid using of the environment variable. A distributive build tool such as rpmbuild or emerge usually sets CFLAGS and LDFLAGS. The problem with incompatible compiler / linker options has been reveal under rpmbuild on CentOS 8 with hardened build enabled (which is so when backtraces are disabled). It is not clear whether we should follow environment variables or values determined by CMake for CFLAGS, CPPFLAGS and LDFLAGS when building a submodule (such as luajit and curl). Let's decide about this later. Part of #4543. Reviewed-by:
Alexander V. Tikhonov <avtikhon@tarantool.org> Reviewed-by:
Igor Munkin <imun@tarantool.org> (cherry picked from commit 0bead600)
-
- Nov 06, 2019
-
-
Kirill Yukhin authored
-
Vladislav Shpilevoy authored
There was a bug that netbox at any schema update called on_connect() triggers. This was due to overcomplicated logic of handling of changes in the netbox state machine. On_connect() was fired each time the machine entered 'active' state, even if its previous states were 'active' and then 'fetch_schema'. The latter state can be entered many times without reconnects. Another bug was about on_disconnect() - it could be fired even if the connection never entered active state. For example, if its first 'fetch_schema' has failed. Now there is an explicit flag showing the machine connect state. The triggers are fired only when it is changed, on 'active' and on any error states. Intermediate states (fetch_schema, auth) do not matter anymore. Thanks @mtrempoltsev for the initial investigation and a draft fix. Closes #4593 (cherry picked from commit d56d869a)
-
- Nov 01, 2019
-
-
Vladislav Shpilevoy authored
Box.session.su() worked like following: check user existence, create its credentials on the stack, check the function, call the function, destroy the credentials, restore the old credentials. After creating the credentials on the stack the function check could raise a Lua error. It led to the credentials object not being destroyed. As a result, user.credentials_list was pointing at invalid memory. Now there is no errors between creating the temporary credentials and its destruction. Closes #4597 (cherry picked from commit 2bb8d1ea)
-
Vladislav Shpilevoy authored
This function is supposed to return NULL on an error. For exceptions there is user_find_by_name_xc. (cherry picked from commit 8b6bdb43)
-
- Oct 31, 2019
-
-
Vladislav Shpilevoy authored
Func_delete() called credentials_destroy() after func->vtab->destroy(). But appeared, that vtab->destroy() is actually delete, and it frees the func object. Now the func's owner credentials are destroyed before the function is freed. Closes #4597 Follow up #2763 (cherry picked from commit 330ea240)
-
- Oct 30, 2019
-
-
Vladislav Shpilevoy authored
Argparse module stores unspecified parameter values as boolean true. It led to a problem, that a command line '--value' with 'value' defined as a number or a string, showed a strange error message: Expected number/string, got "true" Even though a user didn't pass any value. Now it shows 'nothing' instead of '"true"'. That is clearer. Follow up #4076 (cherry picked from commit c214d086)
-
Vladislav Shpilevoy authored
There was a complaint that tarantoolctl --show-system option is very hard to use. It incorrectly parsed passed values, and provided strange errors. tarantoolctl cat --show-system true Bad input for parameter "show-system". Expected boolean, got "true" tarantoolctl cat --show-system 1 Bad input for parameter "show-system". Expected boolean, got "1" tarantoolctl cat --show-system=true Bad input for parameter "show-system". Expected boolean, got "true" First of all, appeared that the complaining people didn't read documentation in 'tarantoolctl --help'. It explicitly says, that '--show-system' should go after a file name, and does not have a value. Secondly, even having taken the documentation into account, the errors indeed look ridiculous. 'Expected boolean, got "true"' looks especially weird. The problem appeared to be with argparse module, how it parses boolean parameters, and how stores parameter values not specified in a command line. All parameters were parsed into a dictionary: parameter name -> value. If a name is alone (no value), then it is boolean true. Otherwise it was always a string value. An attempt to specify an explicit parameter value 'true' led to storing string 'true' in that dictionary. Consequential check for boolean parameters was trivial: type(value) == 'boolean', which was obviously wrong, and didn't pass for 'true' string, but passed for an empty value. Closes #4076 (cherry picked from commit 03f85d4c)
-
Vladislav Shpilevoy authored
Credentials is a cache of user universal privileges. And that cache can become outdated in case user privs were changed after creation of the cache. The patch makes user update all its credentials caches with new privileges, via a list of all creds. That solves a couple of real life problems: - If a user managed to connect after box.cfg started listening port, but before access was granted, then he needed a reconnect; - Even if access was granted, a user may connect after box.cfg listen, but before access *is recovered* from _priv space. It was not possible to fix without a reconnect. And this problem affected replication. Closes #2763 Part of #4535 Part of #4536 @TarantoolBot document Title: User privileges update affects existing sessions and objects Previously if user privileges were updated (via `box.schema.user.grant/revoke`), it was not reflected in already existing sessions and objects like functions. Now it is. For example: ``` box.cfg{listen = 3313} box.schema.user.create('test_user', {password = '1'}) function test1() return 'success' end c = require('net.box').connect(box.cfg.listen, { user = 'test_user', password = '1' }) -- Error, no access for this connection. c:call('test1') box.schema.user.grant('test_user', 'execute', 'universe') -- Now works, even though access was granted after -- connection. c:call('test1') ``` A similar thing happens now with `box.session.su` and functions created via `box.schema.func.create` with `setuid` flag. In other words, now user privileges update is reflected everywhere immediately. (cherry picked from commit 06dbcec597f14fae6b3a7fa2361f2ac513099662) (cherry picked from commit e5149e33ee43cba29af8b395de2b97d446b03979)
-
Vladislav Shpilevoy authored
Struct credentials is a cache of user's universal privileges. It is static and is never changed after creation. That is a problem. If a user privileges are updated, it is not reflected in his existing credentials caches. This patch reworks credentials API so as now this struct is not just a container for several numbers. It is an object with standard methods like create(), destroy(). A credentials object still is not updated together with its source user, but now at least the API allows to fix that. Next patch will link all struct credentials of a user into a list via which the user will be able to keep the credentials up to date. Part of #2763 (cherry picked from commit a8c3ebdbfc97b72832ebc5d87b681a310cce9589) (cherry picked from commit 6f11f2aaed64c80dd490ccbf0edfb9cf437bc00f)
-
- Oct 28, 2019
-
-
Alexander Turenko authored
Added --exclude option (#54). (cherry picked from commit c17c10a4)
-
Alexander Turenko authored
This allows to overcome problems when CMake chooses one toolchain to build tarantool, but a library (libluajit.a or libcurl.a) is built using another (incompatible) toolchain. Fixes #4587. (cherry picked from commit 1eead75e)
-
Alexander Turenko authored
FreeBSD has OpenSSL as part of the base system: libraries are located in /usr/lib, headers are in /usr/include. However a user may install the library into /usr/local/{lib,include} from ports / pkg. In this case tarantool did choose /usr/local version, while libcurl will pick up a base system library. This is fixed by passing --with-ssl option with an argument (/usr/local or /usr if custom -DOPENSSL_ROOT_DIR=<...> is not passed). Now the behaviour is the following. If -DOPENSSL_ROOT_DIR=<...> is passed, then try to use OpenSSL from it. Otherwise find the library in /usr/local and then in /usr. This is right as for tarantool's crypto module as well as for libcurl submodule. There is a flaw here: a user is unable to choose a base system library if a ports / pkg version of OpenSSL is installed. The reason here is that tarantool's crypto module depends on other libraries and -I/usr/local/include may be added to build options. I have no good solution for that, so `cmake . -DOPENSSL_ROOT_DIR=/usr` will give a warning on FreeBSD and `gmake` likely will fail if libraries are of different versions (see cmake/os.cmake comments for more information). See also a [discussion][1] in FreeBSD community about all those /usr and /usr/local problems. There were two other problems that may fail tarantool build on FreeBSD: they are fixed in this commit and described below. First, libcurl's configure script chooses GCC by default if it exists (say, installed from ports / pkg). It is unexpected behaviour when tarantool sources itself are built with clang. Now it is fixed by passing a compiler explicitly to the libcurl's configure script: the library will use base system clang by default or one that a user pass to tarantool's cmake. Side note: GCC has /usr/local/include in its default headers search paths; libcurl's configure script chooses GCC as a compiler and OpenSSL from a base system by default (when CC and --with-ssl=<...> are not set) that leads to OpenSSL header / library mismatch. It is the primary reason of the build fail that was fixed in 1f2338bd ('build: FreeBSD packages installation'). It is not much relevant anymore, because we don't try to link with a base system OpenSSL if /usr/local one exists (however if it is asked explicitly with -DOPENSSL_ROOT_DIR=<...> we'll do, but will give a warning). Anyway, it is important to know such details if we'll change build scripts in a future. Second, backtraces are not supported on FreeBSD, but were enabled if libunwind headers is found. This leads to an error on cmake stage, because of inability to find a right library (this is a bug). Now we disable backtraces on FreeBSD by default even if libunwind is found. See When CC is passed to libcurl's configure script, the new problem opens on Mac OS. CMake chooses XCode toolchain by default (at least on a particular system where I tried it), which requires -isysroot=<SDK_PATH> option to be passed to a preprocessor and a compiler in order to find system headers. See [2] for more information. [1]: https://wiki.freebsd.org/WarnerLosh/UsrLocal [2]: https://developer.apple.com/documentation/xcode_release_notes/xcode_10_release_notes#3035623 Follows up #4490. (cherry picked from commit ea5929db)
-
Vladislav Shpilevoy authored
Before the patch the nil UUID was ignored and a new random one was generated. This was because internally box treats nil UUID as its absence. Now a user will see an explicit message that nil UUID is a reserved value. Closes #4282 (cherry picked from commit a8ebd334)
-
- Oct 24, 2019
-
-
Alexander V. Tikhonov authored
Added Ubuntu 19.10 Eoan Ermine into CI. Close #4583 Reviewed-by:
Alexander Turenko <alexander.turenko@tarantool.org> (cherry picked from commit c6cd2e62)
-
Oleg Babin authored
Before this patch RUN_TESTS condition in Dockerfile.staticbuild was ignored and always was true. However adding of brackets solves only part of problem. If RUN_TESTS is empty `sh -c` returns 1 and build fails. However if we run tests we should fail build if tests are not passed. Ternary logic was rewritten to fair if-else. This patch fixes it and allows build tarantool statically without running tests. @Totktonada: Fixed .gitlab.mk to pass RUN_TESTS environment variable to docker build arguments. Reviewed-by:
Alexander Turenko <alexander.turenko@tarantool.org> (cherry picked from commit 8c85dbc7)
-
- Oct 21, 2019
-
-
Ilya Kosarev authored
Wrap throwing lua_newthread in luaT_newthread using luaT_cpcall to process arising error properly. Closes #4556 (cherry picked from commit 54e23d6d)
-
- Oct 17, 2019
-
-
Vladislav Shpilevoy authored
Console client's eval() method in case of an error at reading from a socket was trying to return a variable declared in a different view scope. Instead, the error should be raised to drop the connection. Reviewed-by:
Alexander Turenko <alexander.turenko@tarantool.org> (cherry picked from commit 89ec1d97)
-
Vladislav Shpilevoy authored
In patch c6bea65f I added a bug - replication/misc leaves a bad value in box.cfg.replication. Before that patch the test was resetting this to empty replication. In my patch I forgot about that, and left there the value {box.cfg.listen, "12345"} This patch cleans it up. Follow up #3760 (cherry picked from commit 399899a1)
-
- Oct 15, 2019
-
-
Alexander Turenko authored
test_run:wait_upstream() and test_run:wait_downstream() now wait until an upstream / a downstream appears. This prevents an attempt to index a nil value when one of those functions are called before a record about a peer appears in box.info.replication. It was observed on replication/show_error_on_disconnect test after commit c6bea65f ('replication: recfg with 0 quorum returns immediately'). Fixes #4563. (cherry picked from commit 18864835)
-
- Oct 12, 2019
-
-
Vladislav Shpilevoy authored
Replication quorum 0 not only affects orphan status, but also, according to documentation, makes box.cfg() return immediately regardless of whether connections to upstreams are established. It was not so before the patch. What is worse, even with non 0 quorum the instance was blocked on reconfiguration for connect timeout seconds, if at least one node is not connected. Now quorum is respected on reconfiguration. On a bootstrap it is still impossible to return earlier than replication_connect_timeout, because nodes need to choose some cluster settings. Too early start would make it impossible - cluster's participants will just start and choose different cluster UUIDs. Closes #3760 (cherry picked from commit c6bea65f)
-
- Oct 09, 2019
-
-
Serge Petrenko authored
A successfully fetched remote instance ballot isn't updated during bootstrap procedure. This leads to a case when different instances choose different masters as their bootstrap leaders. Imagine such a situation. You start instance A without replication set up. Instance A successfully bootstraps. You also have instances B and C both with replication set up to {A, B, C} and replication_connect_quorum set to 3 You first start instance B. It doesn't proceed to choosing a leader until one of the events happens: either the replication_connect_timeout runs out, or instance C is up and starts listening on its port. B has established connection to A and fetched its ballot, with some vclock, say, {1: 1}. B retries connection to C every replication_timeout seconds. Then you start instance C. Instance C succeeds in connecting to A and B right away and bootstraps from instance A. Instance A registers C in its _cluster table. This registration is replicated to instance C. Meanwhile, instance C is trying to sync with quorum instances (which is 3), and stays in orphan mode. Now replication_timeout on instance B finally runs out. It retries a previously unsuccessful connection to C and succeeds. C sends its ballot to B with vclock = {1: 2, 2:0} (in our example), since it has already incremented it after _cluster registration. B sees that C has a greater vclock than A, and chooses to bootstrap from C instead of A. C is orphan and rejects B's attempt to join. B dies. To fix such ungentlemanlike behaviour of C, we should at least include loading status in ballot and prefer fully bootstrapped instances to the ones still syncing with other replicas. We also need to use a separate flag instead of ballot's already existent is_ro, since we still want to prefer loading instances over the ones explicitly configured to be read-only. Closes #4527 (cherry picked from commit dc1e4009)
-
Kirill Yukhin authored
(cherry picked from commit 8749b3e1)
-
- Oct 04, 2019
-
-
Cyrill Gorcunov authored
fiber_stack_destroy() doesn't depend on cord() helper but rather accepts slab cache pointer in arguments, lets make the same in fiber_stack_create() helper, otherwise it looks quite imbalanced. Signed-off-by:
Kirill Yukhin <kyukhin@tarantool.org> (cherry picked from commit fa6d78b397d375d4c18875ac72b3798b2e06a1e8)
-
Cyrill Gorcunov authored
The mprotect() syscall may fail due to heavy memory pressure (say there is no enough resources to split VMAs and etc). In this case we must not proceed without guard page but refuse to create a new fiber. Closes #4541 Signed-off-by:
Kirill Yukhin <kyukhin@tarantool.org> (cherry picked from commit e58dfe07189134297ea956eb70cd7d96e20a6c6e)
-
Cyrill Gorcunov authored
This will allow us to call it from inside fiber_stack_create as we need to clean up resources on error. Part-of #4541 Signed-off-by:
Kirill Yukhin <kyukhin@tarantool.org> (cherry picked from commit 8f1251d3611aa30d5bb6098716e965fd8dc08551)
-
- Oct 01, 2019
-
-
Roman Khabibov authored
(cherry picked from commit 0b9de586)
-
Roman Khabibov authored
Count lines in the json parsing structure. It is needed to print the number of line and column where a mistake was made. Closes #3316 (cherry picked from commit 9f9bd3eb2d064129ff6b1a764140ebef242d7ff7) (cherry picked from commit 53d43160)
-
Vladislav Shpilevoy authored
Code to run main script (passed via command line args, or interactive console) has a footer where it notifies systemd, logs a happened error, and panics. Before the patch that code was unreachable in case of any exception in a main script, because panic happened earlier. Now a happened exception is correctly carried to the footer with proper error processing. A first and obvious solution was replace all panics with diag_set and use fiber_join on the script runner fiber. But appeared, that the fiber running a main script can't be joined. This is because normally it exits via os.exit() which never returns and therefore its caller never dies = can't be joined. The patch solves this problem by passing main fiber diag to the script runner by pointer, eliminating fiber_join necessity. Closes #4382 (cherry picked from commit 157a2d88)
-
- Sep 25, 2019
-
-
Vladislav Shpilevoy authored
Closes #4434 Follow-up #4366 @TarantoolBot document Title: json/msgpack.cfg.encode_deep_as_nil option Tarantool has several so called serializers to convert data between Lua and another format: YAML, JSON, msgpack. YAML is a crazy serializer without depth restrictions. But for JSON, msgpack, and msgpackffi a user could set encode_max_depth option. That option led to crop of a table when it had too many nested levels. Sometimes such behaviour is undesirable. Now an error is raised instead of data corruption: t = nil for i = 1, 100 do t = {t} end msgpack.encode(t) -- Here an exception is thrown. To disable it and return the old behaviour back here is a new option: <serializer>.cfg({encode_deep_as_nil = true}) Option encode_deep_as_nil works for JSON, msgpack, and msgpackffi modules, and is false by default. It means, that now if some existing users have cropping, even intentional, they will get the exception. (cherry picked from commit d7a8942a)
-
Vladislav Shpilevoy authored
Tuple is a C library exposed to Lua. In Lua to translate Lua objects into tuples and back luaL_serializer structure is used. In Tarantool we have several global serializers, one of which is for msgpack. Tuples store data in msgpack, and in theory should have used that global msgpack serializer. But in fact the tuple module had its own private serializer because of tuples encoding specifics such as never encode sparse arrays as maps. This patch makes tuple Lua module use global msgpack serializer always. But how does tuple handle sparse arrays now? In fact, the tuple module still has its own serializer, but it is updated each time when the msgpack serializer is changed. Part of #4434 (cherry picked from commit 676369b1)
-
Vladislav Shpilevoy authored
Msgpack Lua module is not a simple set of functions. It is a global serializer object used by plenty of other Lua and C modules. Msgpack as a serializer can be configured, and in theory its configuration updates should affect all other modules. For example, a user could change encode_max_depth: require('msgpack').cfg({encode_max_depth = <new_value>}) And that would make tuple:update() accept tables with <new_value> depth without a crop. But in fact msgpack configuration didn't affect some places, such as this one. And all the others who use msgpackffi. This patch fixes it, for encode_max_depth option. Other options are still ignored. Part of #4434 (cherry picked from commit 4bb253f7)
-
Vladislav Shpilevoy authored
There are some objects called serializers - msgpack, cjson, yaml, maybe more. They are global objects affecting both Lua and C modules. A serializer have settings which can be updated. But before the patch an update changed only C structure of the serializer. It made impossible to use settings of the serializers from Lua. Now any update of any serializer is reflected both in its C and Lua structures. Part of #4434 (cherry picked from commit fe4a8047)
-
- Sep 20, 2019
-
-
Alexander Turenko authored
The submodule was on 7.65.3 version. This update closes two security problems: https://curl.haxx.se/docs/CVE-2019-5482.html https://curl.haxx.se/docs/CVE-2019-5481.html See also curl-7.66.0 release notes: https://curl.haxx.se/changes.html#7_66_0 Fixes #4502. (cherry picked from commit 4c2d1eff)
-