The name of this method seems ambiguous to me, even where it's used. Is this following an existing pattern?
Perhaps a comment describing what it does may help. Or, is there an alternative name that might be more clear?

I'll add a comment here. But it's equivalent, to the one in zookeeper.py

these should probably be defined as class constants so users can specify the startup modes without having to use literals. this helps if you have an IDE indexing stuff, and lets you prefix in ways that clarify the meaning, e.g. STARTUP_MODE_LISTEN = 'LISTEN'.

kind of unfortunate we haven't migrated to py3 yet, 3.4 added real enums which would be nicer here.

That's what I would have done, but didn't like the naming pattern with all caps. I'll do that.

isn't 8083 basically a constant in this class anyway? what else would we check for?

seems like if we don't want it to be a constant it should be a member variable and not passed in here? other things like _base_url() wouldn't work if you changed it only in the call to this method.

more generally, i wouldn't add flexibility until we need it, and I don't see it being used anywhere afaict. one of the nice things about ducktape is that it sets up environments such that your tests don't need flexibility and make things simpler to write.

if these constants aren't variables somewhere, then this should be a docstring instead of a comment above the method.

i assume this is used instead of start(self, mode='LISTEN') due to naming conflicts? the name's a bit confusing since in fact both LOAD and LISTEN are synchronous, at least to some degree. i'm not sure just overriding with that extra parameter is even an issue. Would

def start(self, mode='LISTEN'):
    self.startup_mode = mode
    super(ConnectServiceBase, self).start()

not work?

The call to the parent method was something that I wasn't sure of ... Thanks!

the drawback of renaming here is that the name start_cmd is used with start/start_node pretty standardly across the rest of this code base

Had to rename because of a weird error:

cmd = self.start_cmd(node, remote_connector_configs)
[INFO:2018-01-15 17:26:38,306]: TypeError: start_cmd() takes exactly 2 arguments (3 given)

Let's revisit in the future. Right now, renaming was the only way to make this call work generically with both standalone and distributed classes.

ack. would probably be good to verify the minimal diff to rename back after we're confident of the current patch (and might reveal more info), but this isn't all that big a deal if it changes, just makes it a bit inconsistent.

is this really what was intended given compatibility? and do none of our tests rely on the existing behavior to wait until that message is logged? i would have expected some that use the REST API (e.g. pretty much anything distributed that submits a connector as one of its first post-start tasks) and would be flaky if we change the default to INSTANT

Ok, written this way is confusing. I got confused too, before I issue the PR. Indeed, the else might correspond to INSTANT but the thing is that the member variable startup_mode is initialized to LISTEN at the top of the class. So it shouldn't be INSTANT in general and in any of the previous tests, unless explicitly set to that.

But given it's confusing, I'll change it.

I think this docstring isn't applying to what you think it is.

String literals occurring immediately after a simple assignment at the top level of a module, class, or __init__ method are called "attribute docstrings".

This docstring is applying to CONNECT_REST_PORT rather than the subsequent STARTUP_MODE_* constants. This is unfortunate about not having enums in py2. You could either document each separately, or put them under a nested class for namespacing.

:) Gotcha

ack. would probably be good to verify the minimal diff to rename back after we're confident of the current patch (and might reveal more info), but this isn't all that big a deal if it changes, just makes it a bit inconsistent.