summaryrefslogtreecommitdiff
path: root/ctdb
Commit message (Collapse)AuthorAgeFilesLines
* ctdb-recoverd: Rename update_local_flags() -> update_flags()Martin Schwenke2020-08-271-6/+6
| | | | | | | | | | | | This also updates remote flags so the name is misleading. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14466 Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com> (cherry picked from commit 4aa8e72d60e92951b35190d2ffcfdb1bfb756609) Autobuild-User(v4-13-test): Stefan Metzmacher <metze@samba.org> Autobuild-Date(v4-13-test): Thu Aug 27 12:11:01 UTC 2020 on sn-devel-184
* ctdb-recoverd: Change update_local_flags() to use already retrieved nodemapsMartin Schwenke2020-08-271-17/+9
| | | | | | | BUG: https://bugzilla.samba.org/show_bug.cgi?id=14466 Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com> (cherry picked from commit 702c7c4934e79a9161fdc59df70df30ae492d89f)
* ctdb-recoverd: Get remote nodemaps earlierMartin Schwenke2020-08-271-7/+7
| | | | | | | | | update_local_flags() will be changed to use these nodemaps. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14466 Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com> (cherry picked from commit 910a0b3b747a987ba69b6a0b6256e964b7d85dfe)
* ctdb-recoverd: Do not fetch the nodemap from the recovery masterMartin Schwenke2020-08-271-1/+10
| | | | | | | | | | | | The nodemap has already been fetched from the local node and is actually passed to this function. Care must be taken to avoid referencing the "remote" nodemap for the recovery master. It also isn't useful to do so, since it would be the same nodemap. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14466 Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com> (cherry picked from commit d50919b0cb28f299c9b6985271b29d4f27c5f619)
* ctdb-recoverd: Change get_remote_nodemaps() to use connected nodesMartin Schwenke2020-08-271-3/+2
| | | | | | | | | | | | | | | | | | The plan here is to use the nodemaps retrieved by get_remote_nodes() in update_local_flags(). This will improve efficiency, since get_remote_nodes() fetches flags from nodes in parallel. It also means that get_remote_nodes() can be used exactly once early on in main_loop() to retrieve remote nodemaps. Retrieving nodemaps multiple times is unnecessary and racy - a single monitoring iteration should not fetch flags multiple times and compare them. This introduces a temporary behaviour change but it will be of no consequence when the above changes are made. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14466 Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com> (cherry picked from commit 762d1d8a9605f97973a2c1176de5d29fcc61d15a)
* ctdb-recoverd: Fix node_pnn check and assignment of nodemap into arrayMartin Schwenke2020-08-271-3/+11
| | | | | | | | | This array is indexed by the same index as nodemap, not the PNN. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14466 Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com> (cherry picked from commit 368c83bfe3bbfff568d14f65e7b1ffa41d5349ac)
* ctdb-recoverd: Add fail callback to assign banning creditsMartin Schwenke2020-08-271-8/+17
| | | | | | | | | | Also drop error handling in main_loop() that is replaced by this change. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14466 Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com> (cherry picked from commit 10ce0dbf1c11eaaab7b28b6bbd014235a36d1962)
* ctdb-recoverd: Add an intermediate state struct for nodemap fetchingMartin Schwenke2020-08-271-2/+11
| | | | | | | | | This will allow an error callback to be added. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14466 Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com> (cherry picked from commit a079ee31690cf7110f46b41989ffcfb83b7626d6)
* ctdb-recoverd: Move memory allocation into get_remote_nodemaps()Martin Schwenke2020-08-271-12/+20
| | | | | | | BUG: https://bugzilla.samba.org/show_bug.cgi?id=14466 Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com> (cherry picked from commit 2eaa0af6160588b6e3364b181d0976477d12b51b)
* ctdb-recoverd: Change signature of get_remote_nodemaps()Martin Schwenke2020-08-271-6/+7
| | | | | | | | | | Change 1st argument to a rec context, since this will be needed later. Drop the nodemap argument and access it via rec->nodemap instead. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14466 Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com> (cherry picked from commit 3324dd272c7dafa92cd9c3fd0af8f50084bcdaaa)
* ctdb-recoverd: Fix a local memory leakMartin Schwenke2020-08-271-0/+1
| | | | | | | | | | | | The memory is allocated off the memory context used by the current iteration of main loop. It is freed when main loop completes the fix doesn't require backporting to stable branches. However, it is sloppy so it is worth fixing. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14466 Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com> (cherry picked from commit d2d90f250214582d7124b8137aa2cf5032b2f285)
* ctdb-recoverd: Basic cleanups for get_remote_nodemaps()Martin Schwenke2020-08-271-16/+23
| | | | | | | | | | | Don't log an error on failure - let the caller can do this. Apart from this: fix up coding style and modernise the remaining error message. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14466 Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com> (cherry picked from commit 52f520d39cd92e1cf2413fd7e0dd362debd6f463)
* ctdb-recoverd: Simplify calculation of new flagsMartin Schwenke2020-08-271-3/+1
| | | | | | | | | | Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com> Autobuild-User(master): Martin Schwenke <martins@samba.org> Autobuild-Date(master): Fri Jul 24 06:03:23 UTC 2020 on sn-devel-184 (cherry picked from commit 5ce6133a75107abdcb9fcfd93bc7594812dc5055)
* ctdb-recoverd: Correctly find nodemap entry for pnnMartin Schwenke2020-08-271-2/+8
| | | | | | Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com> (cherry picked from commit 3654e416770cc7521dcc3c15976daeba37023304)
* ctdb-recoverd: Do not retrieve nodemap from recovery masterMartin Schwenke2020-08-271-16/+2
| | | | | | | | It is already in rec->nodemap. Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com> (cherry picked from commit 9475ab044161e687b9ced3a477746393565b49b1)
* ctdb-recoverd: Flatten update_flags_on_all_nodes()Martin Schwenke2020-08-271-43/+26
| | | | | | | | | | | | | | | | | | The logic currently in ctdb_ctrl_modflags() will be optimised so that it no longer matches the pattern for a control function. So, remove this function and squash its functionality into the only caller. Although there are some superficial changes, the behaviour is unchanged. Flattening the 2 functions produces some seriously weird logic for setting the new flags, to the point where using ctdb_ctrl_modflags() for this purpose now looks very strange. The weirdness will be cleaned up in a subsequent commit. Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com> (cherry picked from commit 0c6a7db3ba84b8355359b0a8c52690b234bb866d)
* ctdb-recoverd: Move ctdb_ctrl_modflags() to ctdb_recoverd.cMartin Schwenke2020-08-273-70/+68
| | | | | | | | This file is the only user of this function. Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com> (cherry picked from commit a88c10c5a9afcf0a3dcadef07dd95af498bfa47a)
* ctdb-recoverd: Improve a call to update_flags_on_all_nodes()Martin Schwenke2020-08-271-1/+1
| | | | | | | | This should take a PNN, not an array index. Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com> (cherry picked from commit b1e631ff929fd87392a80895d1c8d265d9df42dc)
* ctdb-recoverd: Use update_flags_on_all_nodes()Martin Schwenke2020-08-271-12/+6
| | | | | | | | This is clearer than using the MODFLAGS control directly. Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com> (cherry picked from commit 915d24ac12d27c21649d9e64d201d9df9d583129)
* ctdb-recoverd: Introduce some local variables to improve readabilityMartin Schwenke2020-08-271-10/+21
| | | | | | Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com> (cherry picked from commit f681c0e947741151f8fb95d88edddfd732166dc1)
* ctdb-recoverd: Change update_flags_on_all_nodes() to take rec argumentMartin Schwenke2020-08-271-4/+5
| | | | | | | | | This makes fields such as recmaster and nodemap easily available if required. Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com> (cherry picked from commit cb3a3147b7a3a29d7806733791e1fa6ba2e46680)
* ctdb-recoverd: Drop unused nodemap argument from update_flags_on_all_nodes()Martin Schwenke2020-08-271-5/+15
| | | | | | | | | An unused argument needlessly extends the length of function calls. A subsequent change will allow rec->nodemap to be used if necessary. Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com> (cherry picked from commit 6982fcb3e6c940d0047aac3b6bfbc9dfdc8d7214)
* ctdb-tests: Stop cat command failure from causing test failureMartin Schwenke2020-08-121-1/+1
| | | | | | | | | | | | | | In certain circumstance, which aren't obvious, cat(1) can fail when attempting to write a lot of data. This is due to something (probably write(2)) returning EAGAIN. Given that the -v option should only really be used for test debugging, ignore the failure instead of spending time debugging it. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14446 Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com> (cherry picked from commit 3ff8765d04c0fb950b7be4f9a049999aeb08223b)
* ctdb-scripts: Use nfsconf as a last resort get nfsd thread countMartin Schwenke2020-08-102-0/+8
| | | | | | | | | | | | | | | | | | If nfsconf exists then use it as last resort to attempt to extract [nfsd]:threads from /etc/nfs.conf. Invocation of nfsconf requires "|| true" because this script uses "set -e". Add a stub that always fails to at least test this much. RN: Use nfsconf utility for variable values in CTDB NFS scripts BUG: https://bugzilla.samba.org/show_bug.cgi?id=14444 Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com> Autobuild-User(master): Amitay Isaacs <amitay@samba.org> Autobuild-Date(master): Mon Jul 27 07:06:58 UTC 2020 on sn-devel-184 (cherry picked from commit 642dc6ded6426ba2fbf3ac1e5cd71aae11ca245b)
* ctdb-scripts: Use nfsconf as a last resort to set NFS_HOSTNAMEMartin Schwenke2020-08-101-4/+17
| | | | | | | | | | If nfsconf exists then use it as last resort to attempt to extract [statd]:name from /etc/nfs.conf. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14444 Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com> (cherry picked from commit 334dd8cedda6a341e3b89c9adc8102ea5480e452)
* ctdb-tests: Add a new fetch ring test that also checks hot keysMartin Schwenke2020-05-221-0/+161
| | | | | | | | Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com> Autobuild-User(master): Amitay Isaacs <amitay@samba.org> Autobuild-Date(master): Fri May 22 08:05:54 UTC 2020 on sn-devel-184
* ctdb-tests: Update fetch_ring to take database and key on command lineMartin Schwenke2020-05-222-12/+21
| | | | | Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com>
* ctdb-daemon: Fix sorting of hot keysMartin Schwenke2020-05-221-14/+19
| | | | | | | | | | The current code only ever swaps with slot 0. This will only ever happen with slots 0 and 1, so probably never sorts. Replace with qsort(). Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com>
* ctdb-daemon: Add extra logging of hot keysMartin Schwenke2020-05-223-0/+14
| | | | | | | | | | | ctdbd currently only logs when a new hot key is added. If a key gets hotter then nothing new is logged. Log hot key updates when the number of migrations has doubled since the last time that key was logged. Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com>
* ctdb-daemon: Update hot key loggingMartin Schwenke2020-05-221-3/+4
| | | | | | | | | This message indicates that a hot key was added, so say that. After all the hot key slots have been filled the id will always be 0, so don't bother logging it. Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com>
* ctdb-daemon: Fix bug in slot 0 comparison optimisationMartin Schwenke2020-05-221-2/+7
| | | | | | | This is only valid if all slots are in use. Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com>
* ctdb-daemon: Switch some variables to unsignedMartin Schwenke2020-05-222-4/+4
| | | | | | | These should be unsigned but luck is currently on our side. Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com>
* ctdb-daemon: Add separate hot keys array for database statisticsMartin Schwenke2020-05-223-23/+38
| | | | | | | | | | There are 2 reasons for this. Sorting of hot keys is broken and will be changed to an implementation that needs a named (i.e. not anonymous) structure. Also, at least one non-protocol field will be added to facilitate more useful logging. Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com>
* ctdb-build: Fix a typoMartin Schwenke2020-05-221-1/+1
| | | | | Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com>
* ctdb: increase TasksMax limit, the systemd default is just 512Ralph Boehme2020-05-131-0/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | In 2015 systemd introduced a TasksMax which limits the number of processes in a unit: https://lists.freedesktop.org/archives/systemd-devel/2015-November/035006.html The default of 512 may be too low in certain situations leading to vfork() failing with errno=EAGAIN when trying to spawn lock-helper processes. With the default for LockProcessesPerDB being 200 the increased TasksMax limit should cover the problematic scenario. Additional background: the failing vfork()s have been seen on production clusters and were tracked down to being logged in the context of ctdb calling tdb_repack(). Links: https://github.com/systemd/systemd/commit/9ded9cd14cc03c67291b10a5c42ce5094ba0912f https://www.suse.com/support/kb/doc/?id=000015901 https://success.docker.com/article/how-to-reserve-resource-temporarily-unavailable-errors-due-to-tasksmax-setting https://www.percona.com/blog/2019/01/02/tasksmax-another-setting-that-can-cause-mysql-error-messages/ Signed-off-by: Ralph Boehme <slow@samba.org> Reviewed-by: Martin Schwenke <martin@meltin.net> Autobuild-User(master): Martin Schwenke <martins@samba.org> Autobuild-Date(master): Wed May 13 13:30:12 UTC 2020 on sn-devel-184
* ctdb-build: Add messages_dgm build to ctdbAmitay Isaacs2020-05-061-0/+4
| | | | | | | | Signed-off-by: Amitay Isaacs <amitay@gmail.com> Reviewed-by: Volker Lendecke <vl@samba.org> Autobuild-User(master): Amitay Isaacs <amitay@samba.org> Autobuild-Date(master): Wed May 6 01:47:16 UTC 2020 on sn-devel-184
* lib/util: Build genrand for util coreAmitay Isaacs2020-05-061-0/+4
| | | | | | | messages_dgm depends on genrand. Signed-off-by: Amitay Isaacs <amitay@gmail.com> Reviewed-by: Volker Lendecke <vl@samba.org>
* ctdb: Implement CTDB_CONTROL_ECHO_DATAVolker Lendecke2020-04-281-0/+87
| | | | | | | | | | | Testing control: 4 bytes msec delay plus a blob, return the request after the delay. This is an enhanced "ping" which can be used to test asynchronous clients. Doesn't have the full protocol implementation yet Signed-off-by: Volker Lendecke <vl@samba.org> Reviewed-by: Martin Schwenke <martin@meltin.net>
* ctdb-protocol: Add marshalling for control ECHO_DATAVolker Lendecke2020-04-287-2/+83
| | | | | Signed-off-by: Volker Lendecke <vl@samba.org> Reviewed-by: Martin Schwenke <martin@meltin.net>
* ctdb-protocol: Add marshalling for struct ctdb_echo_dataVolker Lendecke2020-04-285-0/+101
| | | | | Signed-off-by: Volker Lendecke <vl@samba.org> Reviewed-by: Martin Schwenke <martin@meltin.net>
* ctdb-protocol: Add new control CTDB_CONTROL_ECHO_DATAVolker Lendecke2020-04-281-0/+8
| | | | | Signed-off-by: Volker Lendecke <vl@samba.org> Reviewed-by: Martin Schwenke <martin@meltin.net>
* ctdb: Fix duplicate ;;Volker Lendecke2020-04-281-1/+1
| | | | | Signed-off-by: Volker Lendecke <vl@samba.org> Reviewed-by: Martin Schwenke <martin@meltin.net>
* ctdb-scripts: Update nfs-ganesha-calloutRenaud Fortier2020-04-231-1/+1
| | | | | | | | | | | | | | On debian buster, this variable doesn't exist anymore. Look at this PR as a reference: https://github.com/gluster/storhaug/pull/30 Signed-off-by: Renaud Fortier <renaud.fortier@fsaa.ulaval.ca> Reviewed-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Andrew Bartlett <abartlet@samba.org> Autobuild-User(master): Martin Schwenke <martins@samba.org> Autobuild-Date(master): Thu Apr 23 08:07:51 UTC 2020 on sn-devel-184
* ctdb: Fix a memleakVolker Lendecke2020-04-171-0/+1
| | | | | | | | | Bug: https://bugzilla.samba.org/show_bug.cgi?id=14348 Signed-off-by: Volker Lendecke <vl@samba.org> Reviewed-by: Martin Schwenke <martin@meltin.net> Autobuild-User(master): Martin Schwenke <martins@samba.org> Autobuild-Date(master): Fri Apr 17 08:32:35 UTC 2020 on sn-devel-184
* ctdb-vacuum: Reschedule vacuum event if VacuumInterval has increasedMartin Schwenke2020-04-071-0/+16
| | | | | | | | | | | | | | | | | | | | | | | | The vacuuming integration tests set VacuumInterval to a very high number to avoid vacuuming collisions. This is done after the cluster is healthy, so Samba will have already been started and vacuuming will already be scheduled *at the default interval* for databases attached by Samba. This means that vacuuming controls used by vacuuming tests can still collide with the scheduled vacuuming events. Add some logic to reschedule a vacuuming event that has fired but where VacuumInterval has increased since it was originally scheduled. The increase in VacuumInterval is used as the time offset for rescheduling the event. Although this changes production behaviour for the convenience of testing, the new behaviour is completely reasonable and obeys the principle of least surprise. Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com> Autobuild-User(master): Amitay Isaacs <amitay@samba.org> Autobuild-Date(master): Tue Apr 7 03:04:57 UTC 2020 on sn-devel-184
* ctdb-vacuum: Store value of VacuumInterval in ctdb_vacuum_handleMartin Schwenke2020-04-071-3/+10
| | | | | | | | No behaviour change. This is final staging to make the next change completely obvious. Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com>
* ctdb-vacuum: Use vacuum_handle local variablesMartin Schwenke2020-04-071-10/+20
| | | | | | | | | | No behaviour change. This just makes future changes clearer by avoiding reformatting (or introducing local variables). Clean up error handling while touching a relevant line. Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com>
* ctdb-recoverd: Avoid dereferencing NULL rec->nodemapMartin Schwenke2020-03-241-4/+4
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | Inside the nested event loop in ctdb_ctrl_getnodemap(), various asynchronous handlers may dereference rec->nodemap, which will be NULL. One example is lost_reclock_handler(), which causes rec->nodemap to be unconditionally dereferenced in list_of_nodes() via this call chain: list_of_nodes() list_of_active_nodes() set_recovery_mode() force_election() lost_reclock_handler() Instead of attempting to trace all of the cases, just avoid leaving rec->nodemap set to NULL. Attempting to use an old value is generally harmless, especially since it will be the same as the new value in most cases. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14324 Reported-by: Volker Lendecke <vl@samba.org> Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com> Autobuild-User(master): Martin Schwenke <martins@samba.org> Autobuild-Date(master): Tue Mar 24 01:22:45 UTC 2020 on sn-devel-184
* ctdb-daemon: Don't allow attach from recovery if recovery is not activeMartin Schwenke2020-03-231-0/+7
| | | | | | | | | | Neither the recovery daemon nor the recovery helper should attach databases outside of the recovery process. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14294 Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com>
* ctdb-daemon: Remove more unused old client database functionsMartin Schwenke2020-03-232-117/+0
| | | | | | | BUG: https://bugzilla.samba.org/show_bug.cgi?id=14294 Signed-off-by: Martin Schwenke <martin@meltin.net> Reviewed-by: Amitay Isaacs <amitay@gmail.com>