[GitHub] storm issue #2199: [STORM-2201] Add dynamic scheduler configuration loading
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2199 @revans2 still +1. Looks like we can merge this now. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #2199: [STORM-2201] Add dynamic scheduler configuration loading
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/2199 @HeartSaVioR you took a look at this patch before, do you want to take another look at it? @Ethanlm could you squash your commits into just one? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #2199: [STORM-2201] Add dynamic scheduler configuration loading
Github user Ethanlm commented on the issue: https://github.com/apache/storm/pull/2199 Tested it again. Should be good now. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #2199: [STORM-2201] Add dynamic scheduler configuration loading
Github user Ethanlm commented on the issue: https://github.com/apache/storm/pull/2199 I am done with testing and I think this patch is ready for review. Now I have `ArtifactoryConfigLoader` supports both `artifactory+http` and `artifactory+https` schemes. @HeartSaVioR @revans2 @jerrypeng Could you please let me know if you have any suggestions on this patch when you get a chance. Thanks very much! I want to explain a bit about `Artifactory` and `ArtifactoryConfigLoader` a bit if you are not familiar with the idea. To use `ArtifactoryConfigLoader`, we can set the uri pointing to a directory or a file on Artifactory server. We can query the REST API `api/storage` to get the metadata of the artifact. If the URI (set at `scheduler.config.loader.uri` ) points to a directory, we can get the metadata like this:  We have `children` which are the files under this directory and `ArtifactoryConfigLoader` will pick the file with the largest lexographic name. It the URI points to a file, the metadata will be like this:  where we can get the "downloadUri" and then download that file. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #2199: [STORM-2201] Add dynamic scheduler configuration loading
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2199 @Ethanlm Sure, please leave a comment when you are done. I'll take a look at further changes too. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #2199: [STORM-2201] Add dynamic scheduler configuration loading
Github user Ethanlm commented on the issue: https://github.com/apache/storm/pull/2199 @HeartSaVioR Yes, please. But this patch is not ready yet. I am going to do some tests manually on ArtifactoryHttpConfigLoader and also add some comments to the source code. Thanks. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #2199: [STORM-2201] Add dynamic scheduler configuration loading
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2199 @Ethanlm I guess you're handling some different issues concurrently. (at least this and locality aware shuffle grouping) If you're not currently working on HttpConfigLoader, I'm OK to file a new issue to address HttpConfigLoader, and keep my +1. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #2199: [STORM-2201] Add dynamic scheduler configuration loading
Github user Ethanlm commented on the issue: https://github.com/apache/storm/pull/2199 @revans2 Do you have any suggestions? Thanks. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #2199: [STORM-2201] Add dynamic scheduler configuration loading
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2199 @Ethanlm Thanks for the information. Looks like we can't expect users to use artifactory. If possible we'd be better to have HttpConfigLoader implementation, but may be not easy to implement same for artifactory like pulling most recent file. We can start with just getting full URL for the config file, and pull it periodically. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #2199: [STORM-2201] Add dynamic scheduler configuration loading
Github user Ethanlm commented on the issue: https://github.com/apache/storm/pull/2199 @HeartSaVioR You may want to check with this [website](https://www.jfrog.com/artifactory/). My understanding is that we can get most recent file from artifactory server. It's not the same with normal http server. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #2199: [STORM-2201] Add dynamic scheduler configuration loading
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2199 @Ethanlm I would like to be clear about `Artifactory` to continue reviewing. I have no idea what is this, but whether it is open specification or proprietary on specific company, if we can provide it to users, we don't worry about HttpConfigLoader. And if we need to define some interfaces for HttpConfigLoader and have to provide the implementation, maybe we could just implement `Artifactory` and let users spin and use it. Please mention with cc. to proper folk(s) when you think you couldn't answer this question. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #2199: [STORM-2201] Add dynamic scheduler configuration loading
Github user Ethanlm commented on the issue: https://github.com/apache/storm/pull/2199 I refactored the code. It simplifies configurations a lot. `getConfigLoader()` will choose `IConfigLoader` implementation based on the scheme of `scheduler.config.loader.uri`. It doesn't require users to set many parameters. `FileConfigLoader` will be used if `file://` scheme is used; `ArtifactoryHttpConfigLoader` if `artifactory+http://`. So we can implement `HttpConfigLoader` for normal `http` scheme (TO DO). Doing some more testing, especially for `ArtifactoryHttpConfigLoader`. `HttpConfigLoader` is to be implemented. Hope to get some suggestions first if any --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #2199: [STORM-2201] Add dynamic scheduler configuration loading
Github user Ethanlm commented on the issue: https://github.com/apache/storm/pull/2199 I rebased the branch and addressed some of your comments. I am still working on the rest of them --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #2199: [STORM-2201] Add dynamic scheduler configuration loading
Github user Ethanlm commented on the issue: https://github.com/apache/storm/pull/2199 @HeartSaVioR Thanks for the review. I will address your comments ASAP but it will take me a little longer to catch up this PR --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #2199: [STORM-2201] Add dynamic scheduler configuration loading
Github user Ethanlm commented on the issue: https://github.com/apache/storm/pull/2199 Hi @HeartSaVioR @revans2, could you please review this when you get a chance? Thanks very much! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
