Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix][build] Fix ps command #22451

Merged
merged 1 commit into from
Apr 9, 2024
Merged

[fix][build] Fix ps command #22451

merged 1 commit into from
Apr 9, 2024

Conversation

nodece
Copy link
Member

@nodece nodece commented Apr 7, 2024

Motivation

58a588ac742d:/pulsar# ./bin/pulsar-daemon start zookeeper
doing start zookeeper ...
starting zookeeper, logging to /pulsar/logs/pulsar-zookeeper-58a588ac742d.log
Note: Set immediateFlush to true in conf/log4j2.yaml will guarantee the logging event is flushing to disk immediately. The default behavior is switched off due to performance considerations.
ps: unrecognized option: p
BusyBox v1.36.1 (2023-11-07 18:53:09 UTC) multi-call binary.

Usage: ps [-o COL1,COL2=HEADER] [-T]

Show list of processes

	-o COL1,COL2=HEADER	Select columns for display
	-T

ps command doesn't include -p option.

Modifications

  • Add procps package to fix the ps command
  • Set the PULSAR_PID_DIR environment variable, and default to /pulsar/logs in the docker environment.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@nodece nodece self-assigned this Apr 7, 2024
@nodece nodece requested a review from lhotari April 7, 2024 03:54
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Apr 7, 2024
@nodece
Copy link
Member Author

nodece commented Apr 7, 2024

I recommend to use the bin/pulsar-daemon instead of supervisord in the integration tests, which can help us to find the pulsar-daemon error.

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

I don't think that bin directory should be writable. We need to modify the scripts so that it writes in some other directory.

@nodece
Copy link
Member Author

nodece commented Apr 7, 2024

What do you think about writing the pid file to data directory?

@lhotari
Copy link
Member

lhotari commented Apr 7, 2024

What do you think about writing the pid file to data directory?

It's better, but just wondering if the script pulsar-daemon should be completely removed.

@lhotari
Copy link
Member

lhotari commented Apr 7, 2024

The default could be the logs directory and letting the user override it with some env variable.

@nodece
Copy link
Member Author

nodece commented Apr 7, 2024

What do you think about writing the pid file to data directory?

It's better, but just wondering if the script pulsar-daemon should be completely removed.

+1, I only recommend keeping the bin/pulsar.

The default could be the logs directory.

Write it to logs dir is a good idea.

letting the user override it with some env variable.

Example: PULSAR_PID_DIR?

@nodece
Copy link
Member Author

nodece commented Apr 7, 2024

The PULSAR_PID_DIR has been defined, we can remove the pulsar-daemon from the docker image, but we can keep this command in the codebase, what do you think?

@lhotari
Copy link
Member

lhotari commented Apr 8, 2024

The PULSAR_PID_DIR has been defined, we can remove the pulsar-daemon from the docker image, but we can keep this command in the codebase, what do you think?

@nodece I don't see the changes. I guess it's fine to keep pulsar-daemon as long as we don't need to make bin writable for using it.

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM

@nodece nodece merged commit 3bdb30c into apache:master Apr 9, 2024
52 of 53 checks passed
@nodece nodece added this to the 3.3.0 milestone Apr 9, 2024
hanmz pushed a commit to hanmz/pulsar that referenced this pull request Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs ready-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants