Skip to content

Add pipeline - #7

Open
neckbosov wants to merge 2 commits into
mainfrom
pipeline
Open

Add pipeline#7
neckbosov wants to merge 2 commits into
mainfrom
pipeline

Conversation

@neckbosov

Copy link
Copy Markdown
Collaborator

No description provided.

@neckbosov
neckbosov requested review from M0nsteR and NekoNe May 28, 2021 22:03
Comment thread internal/interface/httpapi/api.go Outdated
realLinks[i] = lnk.RealLink
}
availabilities, err := getLinksAvailability(realLinks)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Oh, please! Remove this new line! 😃

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

fixed

Comment thread internal/interface/httpapi/api.go Outdated
urlsConcurrency = 4
)

func getLinksAvailability(urls []string) ([]bool, error) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

So, you are making some pipeline running on API call?
And you are handling OS signal from the context of API call handler? Am I right?

Why do you do this in the context of user request in the first place? It could be done completely independently.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'll try to clarify my point. It is very disturbing to see OS signal handling at a random place. What are you trying to achieve here? Will somebody send a signal to cancel exactly this API call? Will you add signal handling in each of API calls?
If you want to have a possibility to turn down pipeline use more idiomatic and clean way. Place OS signals handling near the entry point of you program and use context package to propagate signals.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

ok, i create context for api and move signal handling here

@neckbosov
neckbosov requested a review from NekoNe June 18, 2021 23:23
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