find_redis_address docstring

[dHannasch]

Why are these changes needed?

Currently, find_redis_address() lacks a docstring, and is somewhat complicated and counter-intuitive when reading the code. This adds a specific example of a string find_redis_address() might process.

Related issue number

Checks

• I've run scripts/format.sh to lint the changes in this PR.
• I've included any doc changes needed for https://docs.ray.io/en/master/.
• I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
• Testing Strategy
  • Unit tests
  • Release tests
  • This PR is not tested :(

[edoakes]

Good idea, just a comment about naming.

Also, is it possible to only show the relevant parts of the command that we process or do you think showing the full thing verbatim is necessary? Right now it's hard to scan the docstring to understand what I should be looking for.

[edoakes]

Let's call this --address instead of --redis-address everywhere in the comment (--redis-address is the deprecated name).

[dHannasch]

I've added a note that --redis-address is deprecated, but I think that the fact that this function looks for --redis-address instead of --address is literally the most important thing to know about this function, precisely because it's using the deprecated name.

Right now, if something is going wrong and someone comes poking through this code, that --redis-address sure jumps out, and it's helpful to have something clarifying "If you check ps aux and the command line looks like this, then the problem is not here."

And in the future, when the relevant command lines actually are changed to remove --redis-address --- I mean, this function is going to inevitably break, right? Because in the actual code it is in fact looking for --redis-address. So having a warning here might save some time for the poor developer trying to remove --redis-address.

[edoakes]

I didn't actually realize that we hadn't changed --redis-address internally! This makes sense to me, thanks.

[dHannasch]

Yeah, it sounds like he didn't realize either until he started digging into it: #11735
I don't expect the docstring to help much, but hopefully it'll help at least a little the next time something like #11735 comes up.

[dHannasch]

is it possible to only show the relevant parts of the command that we process or do you think showing the full thing verbatim is necessary? Right now it's hard to scan the docstring to understand what I should be looking for.

Hmm. There are probably three audiences here.

The original purpose was just to add an explanation for people poking through the code looking for places to set traps because they suspect some function in here is not returning the correct address. Parsing a cmdline certainly looks fragile, so it draws the eye, so it's nice to have a docstring explaining what this function is doing.

But as long as I was adding a docstring, I figured I'd to add a warning for passing developers. Maybe this is just me, but when I see something like this I want to add asserts or something to check everything that's in the line, to be sure we're not pulling out --redis-address when the correct address is --redis_address which was also present but ignored.

The big long string is to get across just how hard this problem is. (For one thing, there actually is a --redis_address, and we don't want it.) It's actually very much reduced in its current form; the verbatim string has a bunch of long arguments that make it even harder to eyeball what's going on.

And the third purpose is for developers who come seeking this function specifically because it has broken, which is probably will sooner or later. (If nothing else, as you note, --redis-address is deprecated.) If, for example, the control flow gets rearranged so that those nested commands aren't sitting in front of the raylet command, this function will break, and someone looking at what it's trying to parse and what it's not finding might assume that the problem is that it's looking for --redis_address as --redis-address.

For that hypothetical where the function isn't working and it's not clear why, it seems worth knowing "So exactly what input did this thing get back when it was working?"