[GitHub] [pulsar] addisonj commented on issue #5400: [functions] Allow functions to pass runtime specific options
addisonj commented on issue #5400: [functions] Allow functions to pass runtime specific options URL: https://github.com/apache/pulsar/pull/5400#issuecomment-553758466 @sijie if you have a second to merge this, that would be awesome :) 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [pulsar] addisonj commented on issue #5400: [functions] Allow functions to pass runtime specific options
addisonj commented on issue #5400: [functions] Allow functions to pass runtime specific options URL: https://github.com/apache/pulsar/pull/5400#issuecomment-553586235 rerun integration tests 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [pulsar] addisonj commented on issue #5400: [functions] Allow functions to pass runtime specific options
addisonj commented on issue #5400: [functions] Allow functions to pass runtime specific options URL: https://github.com/apache/pulsar/pull/5400#issuecomment-553489854 rerun integration tests 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [pulsar] addisonj commented on issue #5400: [functions] Allow functions to pass runtime specific options
addisonj commented on issue #5400: [functions] Allow functions to pass runtime specific options URL: https://github.com/apache/pulsar/pull/5400#issuecomment-553165046 @jerrypeng changes made, let me know if you have anything else, glad it shaped up well :) 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [pulsar] addisonj commented on issue #5400: [functions] Allow functions to pass runtime specific options
addisonj commented on issue #5400: [functions] Allow functions to pass runtime specific options URL: https://github.com/apache/pulsar/pull/5400#issuecomment-552625063 @jerrypeng I ended up re-working this a fair amount to be more in-line with some of your ideas. Primarily: - created the `RuntimeCustomizer` class and had the `KubernetesManifestCustomizer` extend that class - the `RuntimeCustomizer` now gets created in the `FunctionRuntimeManager` and is passed to all `RuntimeFactory` classes - reverted the changes to how the `FunctionAuthProvider` gets created to also be back in `FunctionRuntimeManager` To make this work, it required a few changes to some of the interfaces, primarily: - The `FunctionAuthProvider` now takes the `FunctionDetails`, this gives us all the data we need to effectively have the `FunctionAuthProvider` call the `KubernetesManifestCustomizer` so that we could get at the `customRuntimeOptions` which may change the namespace that secrets are created under. Additionally, this seems like a sane thing to do as now implementors of that interface get more data, which could be useful for different types of implementations - Made some small tweaks to the `RuntimeFactory` such that the returned `FunctionAuthProvider` and `RuntimeCustomzer` can be sub-classes of those classes that are specialized by implementing interfaces - Changed the `KubernetesManifestCustomizer` such that it only needs one instance of it as all the data needed to customize is passed via the methods (basically added `FunctionDetails` being passed to all calls) 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [pulsar] addisonj commented on issue #5400: [functions] Allow functions to pass runtime specific options
addisonj commented on issue #5400: [functions] Allow functions to pass runtime specific options URL: https://github.com/apache/pulsar/pull/5400#issuecomment-552048442 Perhaps I am missing something, but I think there is a mismatch here. The `FunctionRuntimeManager` can manage the creation of the `RuntimeCustomizer`, but it couldn't be eagerly in it's constructor or initialization methods. It would need to be during the `addAssignment` and `updateAssignment` methods as that is the first place where we get the function details. In other words, the `FunctionRuntimeManager`'s lifecycle is such that it managed multiple functions and would need to have a factory method to create a `RuntimeCustomizer` for each function. This is essentially the problem with `FunctionAuthProvider` as it currently is created once for the whole runtime and instead needs to be per function. Perhaps the `RuntimeCustomizer` could be responsible for the lifecycle of the `FunctionAuthProvider` as well, but their lifecycles would seem intertwined. Let me know if I am missing something. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [pulsar] addisonj commented on issue #5400: [functions] Allow functions to pass runtime specific options
addisonj commented on issue #5400: [functions] Allow functions to pass runtime specific options URL: https://github.com/apache/pulsar/pull/5400#issuecomment-552046680 I considered such an approach, but wasn't sure if that was something that made sense for other providers. Will take a look at refactoring this to support that. However, I do believe the changes (or som other changes) to KubernetesFunctionAuthProvider are necessary such that the namespace in which the secret is created can be overridden by the customizer. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [pulsar] addisonj commented on issue #5400: [functions] Allow functions to pass runtime specific options
addisonj commented on issue #5400: [functions] Allow functions to pass runtime specific options URL: https://github.com/apache/pulsar/pull/5400#issuecomment-552028052 @sijie @jerrypeng this is ready for another look, let me know if you have any questions! 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [pulsar] addisonj commented on issue #5400: [functions] Allow functions to pass runtime specific options
addisonj commented on issue #5400: [functions] Allow functions to pass runtime specific options URL: https://github.com/apache/pulsar/pull/5400#issuecomment-551971510 rerun java8 tests 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [pulsar] addisonj commented on issue #5400: [functions] Allow functions to pass runtime specific options
addisonj commented on issue #5400: [functions] Allow functions to pass runtime specific options URL: https://github.com/apache/pulsar/pull/5400#issuecomment-548826149 @jerrypeng @sijie I reworked this to account for the changes from #5404, I think it seems pretty reasonable. If you could give another review and let me know if there is anything else you want to see here. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [pulsar] addisonj commented on issue #5400: [functions] Allow functions to pass runtime specific options
addisonj commented on issue #5400: [functions] Allow functions to pass runtime specific options URL: https://github.com/apache/pulsar/pull/5400#issuecomment-548686861 rerun integration tests 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [pulsar] addisonj commented on issue #5400: [functions] Allow functions to pass runtime specific options
addisonj commented on issue #5400: [functions] Allow functions to pass runtime specific options URL: https://github.com/apache/pulsar/pull/5400#issuecomment-548664048 rerun java8 tests 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [pulsar] addisonj commented on issue #5400: [functions] Allow functions to pass runtime specific options
addisonj commented on issue #5400: [functions] Allow functions to pass runtime specific options URL: https://github.com/apache/pulsar/pull/5400#issuecomment-546643173 I started in on re-working this... the changes introduced in #5404 broke one of the assumptions this made, which was that we create new instances of the functionAuthProvider, with a specific one for any overridden namespace. @jerrypeng do you see any problem with pushing the creation of the FunctionAuthProvider (via reflection) into the KubernetesRuntimeFactory? Or should we move the namespace used elsewhere? This provides a lot more flexibility to end users as being able to isolate various tenants to their own k8s namespace for functions can be use for finer grain isolation of underlying resources. Would love to get this one through this week, but just want to make sure I am aligned in how to go about this 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [pulsar] addisonj commented on issue #5400: [functions] Allow functions to pass runtime specific options
addisonj commented on issue #5400: [functions] Allow functions to pass runtime specific options URL: https://github.com/apache/pulsar/pull/5400#issuecomment-544696751 okay @jerrypeng I changed that, will see if I can get the tests happy, let me know if you have any other feedback! 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [pulsar] addisonj commented on issue #5400: [functions] Allow functions to pass runtime specific options
addisonj commented on issue #5400: [functions] Allow functions to pass runtime specific options URL: https://github.com/apache/pulsar/pull/5400#issuecomment-542912063 This conflicts with #5398, will need rebased if that is merged first 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: us...@infra.apache.org With regards, Apache Git Services
