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

router.sh is missing in Druid Distribution #15547

Merged
merged 2 commits into from
Dec 15, 2023
Merged

Conversation

aleksi75
Copy link
Contributor

@aleksi75 aleksi75 commented Dec 12, 2023

Every time we roll out a new version of Druid on our cluster, I recognize that the script for starting the router process is missing. So I added it =)

Description

Added router.sh for starting the router process.

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

Every time we roll out a new version of Druid on our cluster, I recognize that the script for starting the router process is missing. So I added it =)
@kfaraz
Copy link
Contributor

kfaraz commented Dec 15, 2023

Thanks for the change, @aleksi75 ! I guess adding a router.sh makes sense as there are existing scripts for other services.

But I am curious as to how you are currently deploying your services. These service-specific scripts are not really used a lot. If you refer to the Druid docs, we typically recommend using the start-druid or run-druid scripts for all of quickstart, single-server deployment and clustered deployment.

Copy link
Member

@xvrl xvrl left a comment

Choose a reason for hiding this comment

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

I'm fine adding this script for consistency. @kfaraz if you have concerns about these scripts in general, then I would suggest we deprecate them and remove them altogether in favor of something else.

@xvrl xvrl merged commit 90af71b into apache:master Dec 15, 2023
83 checks passed
@aleksi75
Copy link
Contributor Author

@kfaraz @xvrl
That is easy to tell. We have been running Druid since version 0.8 or 0.9. In the old days, these service-specific scripts were not only common, but I think they were the only way to start up all the services. In the newer distributions, these 'old' scripts exist, so we never change our way to start/stop services. Well, I know the documentation and the new 'way', but I/we ignored it completely. =) And we will do that in the future. We configure JVM settings, RUNTIME settings and which services are running or not running on the nodes for ourselves to match our hardware and cluster. For us, there is no need for fancy scripts, we just need a script to start and stop a service, and that is what these 'old' shell scripts are doing. Please, keep it simple for those who want it simple =)

@xvrl
Copy link
Member

xvrl commented Dec 15, 2023

thanks for the context @aleksi75. I would ask @kfaraz to keep that feedback in mind if they decide to change anything.

@kfaraz
Copy link
Contributor

kfaraz commented Dec 16, 2023

Thanks for the explanation, @aleksi75 .

@xvrl , the scripts themselves seem harmless enough, I was just curious to know how @aleksi75 is using them. We don't really have any plans to remove them at the moment, but they do seem redundant as they just invoke node.sh with the service name as an argument.

@LakshSingla LakshSingla added this to the 29.0.0 milestone Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants