You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2021/05/06 14:42:09 UTC
[GitHub] [apisix-dashboard] fregie opened a new issue #1840: manager-api: Why putting the feature of daemon in core code?
fregie opened a new issue #1840:
URL: https://github.com/apache/apisix-dashboard/issues/1840
When I reading the code of manager-api,it take me a lot of time understanding the moudle of daemon and cmd,but these are not the core features of manager-api.Why don't we keep the core code less and simple?
Maybe it's a good idea taking this feature out of manager-api as a stand-alone tool to manage the manager-api service.And manager-api will only be a executable file that launching a http/https service when executing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] fregie commented on issue #1840: manager-api: Why putting the feature of daemon in core code?
Posted by GitBox <gi...@apache.org>.
fregie commented on issue #1840:
URL: https://github.com/apache/apisix-dashboard/issues/1840#issuecomment-834311829
In file `api/cmd/service.go` and functions `newxxxCommand()` in `api/cmd/manageapi.go`.
I don't think the code in these two files is necessary be added in the core code.It's fine just invoke `manageAPI()` in `manageapi.go` starting the service,and use a stand-alone script or something else to manage this service.
On the other hand, this feature is redundant if it runs in a container.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] fregie closed issue #1840: manager-api: Why putting the feature of daemon in core code?
Posted by GitBox <gi...@apache.org>.
fregie closed issue #1840:
URL: https://github.com/apache/apisix-dashboard/issues/1840
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] fregie commented on issue #1840: manager-api: Why putting the feature of daemon in core code?
Posted by GitBox <gi...@apache.org>.
fregie commented on issue #1840:
URL: https://github.com/apache/apisix-dashboard/issues/1840#issuecomment-835437048
@bisakhmondal
I have an immature draft,maybe we can just split manager-api into two part
One is only a executable file launching the http service with core logic
The another also use `takama/daemon` but it invokes this executable file
In this way,we can also use commend like `xxx strart`,`xxx stop` etc,and it can also decouple the service management function from the core logic
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] nic-chen commented on issue #1840: manager-api: Why putting the feature of daemon in core code?
Posted by GitBox <gi...@apache.org>.
nic-chen commented on issue #1840:
URL: https://github.com/apache/apisix-dashboard/issues/1840#issuecomment-839410455
> @starsz
> I can have a try.
> Assigne this to me?
looking forward to it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] fregie commented on issue #1840: manager-api: Why putting the feature of daemon in core code?
Posted by GitBox <gi...@apache.org>.
fregie commented on issue #1840:
URL: https://github.com/apache/apisix-dashboard/issues/1840#issuecomment-837541844
I can have a try.
Assigne this to me?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] bisakhmondal commented on issue #1840: manager-api: Why putting the feature of daemon in core code?
Posted by GitBox <gi...@apache.org>.
bisakhmondal commented on issue #1840:
URL: https://github.com/apache/apisix-dashboard/issues/1840#issuecomment-835418522
Hii @fregie, it's nice to see your concern :)
Could you please elaborate on the "standalone tools" part you mentioned?
If it is writing unit files corresponding to the different os-specific service managers, that's exactly what getting handled by the `takama/daemon` [[ref](https://github.com/takama/daemon/blob/496d69192531c6cb1004380a0be0fcf2031b06f4/daemon_linux_systemd.go#L214-L227)] internally. Also if we provide raw unit files, we have to document the steps, from copying the unit file to commands required reloading the daemon manager for registering the service for a list of OS's and also we have to manually manage the lifecycle of those unit files ourselves. The same goes for the shell scripts too.
>we can also use a library to do this to avoid spending a lot of time of us dealing with it.
Sorry, didn't get this. Do you feel we missed something or works left to be done?
Yes, that's the only instance we went with such a trade-off, yet it doesn't touch any of the core logic. Just FYI, if we are running on some hypothetical OS where there is no known service manager `manager-api` will work fine if no commands related to the service is used.
I just stated our motive behind this current approach but new changes are always welcome :)
Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] starsz commented on issue #1840: manager-api: Why putting the feature of daemon in core code?
Posted by GitBox <gi...@apache.org>.
starsz commented on issue #1840:
URL: https://github.com/apache/apisix-dashboard/issues/1840#issuecomment-837874290
> @starsz
> I can have a try.
> Assigne this to me?
Cool.Thank you very much.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] bisakhmondal commented on issue #1840: manager-api: Why putting the feature of daemon in core code?
Posted by GitBox <gi...@apache.org>.
bisakhmondal commented on issue #1840:
URL: https://github.com/apache/apisix-dashboard/issues/1840#issuecomment-835449513
> @bisakhmondal
> I have an immature draft,maybe we can just split manager-api into two part
> One is only a executable file launching the http service with core logic
> The another also use `takama/daemon` but it invokes this executable file
> In this way,we can also use commend like `xxx strart`,`xxx stop` etc,and it can also decouple the service management function from the core logic
Oh, you mean another binary that simply kickstarts the plain old `manager-api` as a service. Thanks for the clarification.
I have a neutral feeling about this approach. Wondering, won't it impact the user experience🤔? Let's hear from the others too.
Pinging @nic-chen, @starsz
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] bisakhmondal commented on issue #1840: manager-api: Why putting the feature of daemon in core code?
Posted by GitBox <gi...@apache.org>.
bisakhmondal commented on issue #1840:
URL: https://github.com/apache/apisix-dashboard/issues/1840#issuecomment-834726990
Hi @fregie, sorry to hear that and thank you for elaborating the issue.
The code had been updated keeping 3 things in mind.
1. Cross-platform support for daemonizing Manager API.
2. Feature-rich CLI experience.
3. And also a good codebase readability.
If we ignore the service part, it's just a CLI command which invokes the `manageAPI` method which handles the core logic without introducing any service-related stuff. And rest `newxxxCommand` commands are just simple addons subcommand attached with the main CLI command (Actually, it's the way the package `spf13/cobra` works, the CLI management library we are using).
Thanks for the recommendation but I think standalone scripts/service unit files focussed on each OS (Linux, Windows, OSX) would make things even messier.
>On the other hand, this feature is redundant if it runs in a container.
Freely use `./manager-api` as an entry point :)
Hope it helps. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] starsz commented on issue #1840: manager-api: Why putting the feature of daemon in core code?
Posted by GitBox <gi...@apache.org>.
starsz commented on issue #1840:
URL: https://github.com/apache/apisix-dashboard/issues/1840#issuecomment-836925677
> I have an immature draft,maybe we can just split manager-api into two part
If so, I think we can use `supervisor` instead.
IMO, I think the feature of the daemon is very good. The reason is talked about before.
Thanks for the @bisakhmondal. 😊.
If you had met any problem while reding the code.
You can create an issue to discuss it or can you help to improve the code readability.
That would be more interesting if we can keep this feature and improve the code readability.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] starsz commented on issue #1840: manager-api: Why putting the feature of daemon in core code?
Posted by GitBox <gi...@apache.org>.
starsz commented on issue #1840:
URL: https://github.com/apache/apisix-dashboard/issues/1840#issuecomment-834571484
Maybe you can refer this Issue： https://github.com/apache/apisix-dashboard/issues/842
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] fregie commented on issue #1840: manager-api: Why putting the feature of daemon in core code?
Posted by GitBox <gi...@apache.org>.
fregie commented on issue #1840:
URL: https://github.com/apache/apisix-dashboard/issues/1840#issuecomment-834973854
@bisakhmondal Thank you for explanation.
I don't think use a standalone tool would make things messier.Because both ways need to focus on each OS (Linux, Windows, OSX),just the current way using library `takama/daemon` dealt with this problem,we can also use a library to do this to avoid spending a lot of time of us dealing with it.
And I don't think it's a good idea deal with system-level differences in core code.This will bring more uncertainty to our core logic.
Like this:
```golang
if runtime.GOOS == "darwin" {
d, err = daemon.New("apisix-dashboard", "Apache APISIX Dashboard", daemon.GlobalDaemon)
} else {
d, err = daemon.New("apisix-dashboard", "Apache APISIX Dashboard", daemon.SystemDaemon)
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] nic-chen commented on issue #1840: manager-api: Why putting the feature of daemon in core code?
Posted by GitBox <gi...@apache.org>.
nic-chen commented on issue #1840:
URL: https://github.com/apache/apisix-dashboard/issues/1840#issuecomment-834283935
hi, @fregie could you point out the files and lines of the code? Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-dashboard] fregie edited a comment on issue #1840: manager-api: Why putting the feature of daemon in core code?
Posted by GitBox <gi...@apache.org>.
fregie edited a comment on issue #1840:
URL: https://github.com/apache/apisix-dashboard/issues/1840#issuecomment-837541844
@starsz
I can have a try.
Assigne this to me?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org