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

Network switch mangos to nats #306

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

Conversation

esuwu
Copy link
Contributor

@esuwu esuwu commented Nov 22, 2024

No description provided.

Comment on lines 52 to 54
tools.StringVarFlagWithEnv(&c.nanomsgPubSubURL, "nano-msg-pubsub-url",
tools.StringVarFlagWithEnv(&c.natsPubSubURL, "nano-msg-pubsub-url",
"ipc:///tmp/discord/nano-msg-nodemon-pubsub.ipc", "Nanomsg IPC URL for pubsub socket")
tools.StringVarFlagWithEnv(&c.nanomsgPairURL, "nano-msg-pair-discord-url",
tools.StringVarFlagWithEnv(&c.natsPairURL, "nano-msg-pair-discord-url",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe also rename flag names?

if err != nil {
return err
}
// todo fix this. send (topic, handlerFunc) into this function
Copy link
Member

Choose a reason for hiding this comment

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

TODO

if err != nil {
return errors.Wrap(err, "failed to subscribe to alert")
}
// todo fix this. send (topic, handlerFunc) into this function
Copy link
Member

Choose a reason for hiding this comment

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

TODO

if err != nil {
return errors.Wrap(err, "failed to unsubscribe from alert")
}
// TODO fix this
Copy link
Member

Choose a reason for hiding this comment

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

What exactly should be fixed?

Comment on lines -218 to +222
tools.StringVarFlagWithEnv(&c.nanomsgPubSubURL, "nano-msg-pubsub-url",
"ipc:///tmp/nano-msg-pubsub.ipc", "Nanomsg IPC URL for pubsub socket")
tools.StringVarFlagWithEnv(&c.nanomsgPairTelegramURL, "nano-msg-pair-telegram-url",
"", "Nanomsg IPC URL for pair socket")
tools.StringVarFlagWithEnv(&c.nanomsgPairDiscordURL, "nano-msg-pair-discord-url",
"", "Nanomsg IPC URL for pair socket")
tools.StringVarFlagWithEnv(&c.natsMessagingURL, "nats-msg-pubsub-url",
"nats://127.0.0.1:4222", "Nats URL for pubsub socket")
tools.DurationVarFlagWithEnv(&c.natsTimeout, "nats-server-timeout",
server.AUTH_TIMEOUT, "Nanomsg IPC URL for pair socket")
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add an option to run nats server in the nodemon process? What do you think? Or better to have it as another process?

if !s.ReadyForConnections(cfg.connectionTimeoutDefault) {
logger.Fatal("NATS server is not ready for connections")
}
logger.Info(fmt.Sprintf("NATS Server is running on host %v, port %d", host, port))
Copy link
Member

Choose a reason for hiding this comment

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

On my mind it's better to print message as NATS Server is running on {host}:{port}

logger.Error("failed to handle bot request", zap.Error(handleErr))
return
}
respndErr := request.Respond(response)
Copy link
Member

Choose a reason for hiding this comment

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

respondErr

default:
logger.Error("Unknown request type", zap.Int("type", int(t)), zap.Binary("message", msg))
}
return nil
// nats considers a message delivered only if there was a not nil response
return []byte(okMessage), nil
Copy link
Member

Choose a reason for hiding this comment

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

So this message can be considered as stub? What if send non nil slice instead of "ok" string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot from 2024-12-05 22-58-33
nats expects a non nil message unfortunately, so there's gotta be something

numberOfStringsAfterSlash = 2
)

func ParseHostAndPortFromURL(natsPubSubURL string) (string, int, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use url.Parse with net.SplitHostPort instead of this? (link)

Copy link
Member

Choose a reason for hiding this comment

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

Maybe add Dockerfile for this service?

@nickeskov
Copy link
Member

I forgot to say, that nats topics should be network-specific. I mean, that one nats broker can be used for multiple monitoring services.

"ipc:///tmp/nano-msg-nodemon-pair.ipc", "Nanomsg IPC URL for pair socket")
tools.StringVarFlagWithEnv(&c.natsPubSubURL, "nats-pubsub-url",
"nats://127.0.0.1:4222", "NATS server URL for pubsub messaging")
tools.StringVarFlagWithEnv(&c.natsPairURL, "nats-pair-discord-url",
Copy link
Member

Choose a reason for hiding this comment

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

Is it still necessary for discord?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can still type some commands as far as I remember

if err != nil {
return err
}
// err := dscBot.subSocket.SetOption(mangos.OptionSubscribe, []byte{byte(alertType)}).
Copy link
Member

Choose a reason for hiding this comment

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

Comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The file you viewed is outdated

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.

2 participants