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

Instacollector Kafka 1.0.0 #9

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Instacollector Kafka 1.0.0 #9

wants to merge 8 commits into from

Conversation

pokharelbinayak
Copy link

version 1.0.0

kafka/README.md Outdated
* The term "VM" in environment of script `cluster_collector.sh` means if running in kernel.

# Design info:
There are two scripts used in instacollector tool for kafka.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you go through and capitalise the product names in here and the scripts? Kafka, Docker, etc.

kafka/README.md Outdated
# Execution settings:
The `cluster_collector.sh` has setting of connecting to cluster nodes using the provided ssh key file or an id file.

If the ssh key has passphrase enabled then please use `ssh-agent` & `ssh-add` commands to add the passphrase before running `cluster_collector.sh` script.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add an example of how to do that?

kafka/README.md Outdated
Note:
* This won't work on versions before kafka 2
* User is requested to change the path values in `node_collector.sh` before running any of the scripts, as by default the script uses `Kafka configuration file locations`, `data directory location`, and other setting locations as per **Apache Kafka** default setup.
* The term "VM" in environment of script `cluster_collector.sh` means if running in kernel.
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you mean by this?

kafka/README.md Outdated
```
2. `cluster_collector.sh`: to be executed on a machine connected to Kafka cluster e.g. user laptop with a running docker or a running VM. It executes node_collector.sh on each Kafka node using ssh. The cluster_collector.sh requires 4 user inputs :
```
* Enter your kafka environment (vm/docker) :
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems to make more sense asking how we want to connect - SSH/Docker.


#id_file='/Users/rahulchakrabarty-instaclustr/.ssh/rahulchakrabarty-instaclustr'
elif [[ "${kenv}" == "docker" ]]; then
read -p "Enter docker home directory :" docker_home
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not clear to me as a user what is this directory supposed to be? It looks like it's used further down as a temporary location to copy the node_collector.sh script to and run from. You should specify that here - otherwise people are going to assume you are asking for a directory on the host machine. Also, do we actually need the user to specify it? Can we just use /tmp, ~, something else?

#Collect environment info (VM/docker)
read -p "Enter your kafka environment (vm/docker) :" kenv

if [[ "${kenv}" == "vm" ]]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems to make more sense asking how we want to connect - SSH/Docker.


if [[ "${kenv}" == "vm" ]]; then
#Collect user info.
read -p "Enter username for login on Kafka cluster nodes (Press Enter for default admin) :" user
Copy link
Collaborator

Choose a reason for hiding this comment

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

Specify SSH username


#user='rahulchakrabarty'

read -p "Enter Identity file path:" id_file
Copy link
Collaborator

Choose a reason for hiding this comment

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

Specify SSH identity file

fi
echo "Copying file node_collector.sh to container"
docker cp ./node_collector.sh $peer:$docker_home/
docker exec $peer /bin/bash -c "sh $docker_home/node_collector.sh $peer $config_file" &
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be docker exec $peer sh "$docker_home/node_collector.sh $peer $config_file" &? What happens if the container doesn't have bash?

You're also not providing the -ip / --ip or -c / --command-config flags. Config file will never be used, and IP will be set to the default hostname output in the node_collector script.

if [[ -z "$peer" ]]; then
break
fi
ssh -i $id_file $user@$peer "bash -s" < node_collector.sh $peer $config_file &
Copy link
Collaborator

Choose a reason for hiding this comment

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

You're not providing the -ip / --ip or -c / --command-config flags. Config file will never be used, and IP will be set to the default hostname output in the node_collector script.

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.

4 participants