Skip to content

Fail accumulo-service stop/kill when no service name is given - #6537

Merged
DomGarguilo merged 1 commit into
apache:2.1from
DomGarguilo:emptyServiceName
Sep 17, 2026
Merged

DomGarguilo merged 1 commit into
apache:2.1from
DomGarguilo:emptyServiceName

Conversation

@DomGarguilo

Copy link
Copy Markdown
Member

Running accumulo-service manager stop without --all or an instance name silently does nothing. The script:

  • prints Stopping service process: with a blank name,
  • looks for the pid file accumulo-.pid, which doesn't exist,
  • exits 0.

The manager keeps running even though the command reported success. A later start then says manager_default_1 already running. kill has the same problem, with no output at all.

This PR adds a check in function main() before stop/kill are dispatched. When all of these are true:

  1. no service name was given
  2. --all was not passed
  3. the script is not in cluster mode (ACCUMULO_CLUSTER_ARG unset, so calls from accumulo-cluster are unaffected)

it exits 1 with this message, followed by the usage text:

Invalid arguments: 'stop' requires --all or a service (e.g. manager_default_1; see 'manager list')

Nothing is stopped, and the user is told what to pass instead.

Also:

  • The usage line changes from stop [--all | []] to stop [--all | ] (same for kill), so the name no longer looks optional.
  • invalid_args now prints its "Invalid arguments" line to stderr, matching the usage text it already sent there.

@DomGarguilo DomGarguilo added this to the 4.0.0 milestone Sep 15, 2026
@DomGarguilo DomGarguilo self-assigned this Sep 15, 2026
Comment thread assemble/bin/accumulo-service Outdated
if [[ -z $command_name ]]; then
invalid_args "<command> cannot be empty"
fi
if [[ $command_name =~ ^(stop|kill)$ && $all_flag != 'true' && -z $ACCUMULO_CLUSTER_ARG && -z $service_name ]]; then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason not to add this arg check in the logic for the stop_service and kill_service functions?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the absence of a more specific parameter should imply 'all'. I'm thinking about other scripts and how they behave for consistency. For example, if you run accumulo-cluster stop, then entire cluster is stopped. The user has to supply arguments for a more targeted action. Maybe we should do the same here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally this ability was added to target names of processes that had been left over from previous cluster actions.
For example: tserver_default_4 would be running but not targetable by stop/kill since the cluster.yaml had been updated to only run 2 tserver processes.

I'm hesitating about adding an easy way to stop all services without forcing the user to be intentional.
accumulo-cluster stop makes sense because cluster implies that multiple nodes/services are being targeted.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea it seems safer force the user to be more explicit when the operation is destructive.

@ddanielr ddanielr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behavior change is beneficial. I'd switch this PR to the 2.1 branch though.
There was a good bit of work done to make sure the control scripts matched between 2.1 and main.

@DomGarguilo
DomGarguilo changed the base branch from main to 2.1 September 16, 2026 19:45
@DomGarguilo

Copy link
Copy Markdown
Member Author

I moved things into stop_service and kill_service and targeted 2.1 in 9901879

@DomGarguilo
DomGarguilo merged commit ec91052 into apache:2.1 Sep 17, 2026
@DomGarguilo
DomGarguilo deleted the emptyServiceName branch September 17, 2026 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants