[GitHub] [geode] aaronlindsey commented on issue #4038: GEODE-7184: Add function executions timer

["aaronlindsey (GitHub)" <gi...@apache.org>]

The following concerns/issues still need to be addressed for this PR:
1. With our changes, `FunctionService.getFunction(String)` will now return a `TimingFunction` instead of the function that that was passed to `FunctionService.registerFunction(Function)`. This could be a problem for users if they are doing an explicit cast after getting the function, like `(MyFunction) FunctionService.getFunction("MyFunction")`. Is this a breaking change? I have a thread going on the dev list about this topic.
2. The PR is failing `AnalyzeSerializablesJUnitTest.testSerializables` because we added a new Serializable class, `TimingFunction`. We need to determine if this class can actually be serialized. If it cannot, we just need to add it to `excludedClasses.txt` and we're done. If it can be serialized, we need to consider the implications. One possible way to avoid backwards compatibility issues would be to override the serialization behavior to replace the serialized form of `TimingFunction` with that of its inner function. After considering the implications, adding the class to `sanctioned-geode-core-serializables.txt` will fix the failing test.
3. Many D-unit tests are failing due to `ClassCastException` and `AssertionError` being thrown because we are now returning `TimingFunction` instead of the function that was passed to `FunctionService.registerFunction(Function)`. We verified that these exceptions are all thrown from test code and not product code. We need to update the test code to expect an instance of `TimingFunction` and know how to get the inner function from the `TimingFunction`.

[ Full content available at: https://github.com/apache/geode/pull/4038 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org