search

valkey-io / valkey

A flexible distributed key-value datastore that is optimized for caching and other realtime workloads.
https://valkey.io
Other
17.43k stars 656 forks source link

Fail `CLIENT IMPORT-SOURCE` When Import Mode is Disabled and Reflect Client Import Source State in `CLIENT LIST` #1350

Open PingXie opened 1 day ago

PingXie commented 1 day ago 
          I think we should clean it up though. Also, shall we update `CLIENT LIST` output to reflect the import source mode?

_Originally posted by @hpatro in https://github.com/valkey-io/valkey/pull/1185#discussion_r1851049362_

PingXie commented 1 day ago

Some random thoughts for consideration

  1. should we make import-mode a debug config (DEBUG_CONFIG)? there is a OOM risk for server operating in this mode so if the server gets OOM killed, it would be good to tell whether this mode was left on accidentally from the crash report.

  2. along the same OOM risk line, can we consider ref-counting "import-source" clients and automatically turn on/off the server "import-mode" based on the number of such clients? This would eliminate the need of explicit clean up (to clear "import-mode" upon the completion of the import). And then we avoid introducing a new server config.

+@soloestoy

soloestoy commented 1 day ago
  1. should we make import-mode a debug config (DEBUG_CONFIG)? there is a OOM risk for server operating in this mode so if the server gets OOM killed, it would be good to tell whether this mode was left on accidentally from the crash report.

Good idea.

  1. along the same OOM risk line, can we consider ref-counting "import-source" clients and automatically turn on/off the server "import-mode" based on the number of such clients? This would eliminate the need of explicit clean up (to clear "import-mode" upon the completion of the import). And then we avoid introducing a new server config.

This is indeed an effective method to "automatically" toggle import-mode, and it can save a configuration.

However, there are a few concerns:

  1. When the "import-source" is 0, valkey automatically exits import-mode, but we don't know whether the source has truly exited or if there is an exceptional disconnection. If the disconnection is due to network issues, the imported data might be expired and deleted upon reconnection.
  2. Enabling the server's status by the client feels quite risky to me. Although I haven't encountered a real-world example yet, intuitively, I think using a configuration would be safer. An explicit configuration would more clearly define its scope and method of use.
PingXie commented 13 hours ago
  1. Enabling the server's status by the client feels quite risky to me. Although I haven't encountered a real-world example yet, intuitively, I think using a configuration would be safer. An explicit configuration would more clearly define its scope and method of use.

I am thinking more in the context of managed services where a user wants to import data into a managed cluster. My main concern is the (user) client never coming back and turning off the import mode on the server. The server then ends up growing its memory footprint unbounded. Making this command a superuser command wouldn't help much either IMO. So how can a server protects itself from a potentially buggy client?

  1. When the "import-source" is 0, valkey automatically exits import-mode, but we don't know whether the source has truly exited or if there is an exceptional disconnection. If the disconnection is due to network issues, the imported data might be expired and deleted upon reconnection.

What if we allow some delay, say 5 mins, before turning off the import mode on the server, after the last "import-source" client drops? This would allow the client to connect back in case of network issues without breaking the "import" semantics while at the same time prevents the memory footprint from growing unbounded on the server?

Just brainstorming ...