This proposal contains a number of improvements to the function service to improve usability issues.
Table of Contents |
---|
Improvements
Minor changes
There are some minor improvements that already have JIRAs. We should just apply the changes described in these jiras.
Jira | ||
---|---|---|
|
Usability issues
Exceptions are handed to ResultCollector.addResult, causing ClassCastExceptions
On the ResultSender, there is a method to return exceptions using sendException.
However, the ResultCollector only has addResult(). Further, ResultCollector has generic types
Code Block |
---|
public interface ResultCollector<T,S> {
public void addResult(DistributedMember memberID, T resultOfSingleExecution); |
Since the type of T is the user result type, trying to call add result will almost invariably result in the ClassCastException.
See
Jira | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
Must use ResultCollector returned from FunctionService.execute
Below, we have to use the collector returned by FunctionService.execute, rather than just calling getResult on our own result collector. We run into problems otherwise, maybe because exceptions are not handed to the user supplied result collector? In any case, its unintuitive that this returns a different ResultCollector than the one supplied by the user.
Example:
Code Block |
---|
ResultCollector<TopEntriesCollector, TopEntries> rc = (ResultCollector<TopEntriesCollector, TopEntries>) FunctionService.onRegion(region)
.withArgs(context)
.withCollector(collector)
.execute(LuceneFunction.ID);
//This doesn't work
TopEntries entries = collector.getResult()
//This is what you have to do
TopEntries entries = rc.getResult();
|
Function invoked with onMember cannot be unit tested, because our API requires using a singleton cache
FunctionContext does not contain a cache. That means any function that you execute with onMember must contain code like this:
Code Block |
---|
public void execute(FunctionContext context) {
Cache cache = CacheFactory.getAnyInstance();
//...do work with the cache
} |
This makes it impossible to write pure unit tests for the function code that mock the cache.
Jira | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
Jira | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
Jira |
---|
Jira | showSummary | false|||||||
---|---|---|---|---|---|---|---|---|
|
Functions invoked with onRegion always have to cast the FunctionContext to RegionFunctionContext
|
Functions executed on a region should implement a different interface
...
Any function that is executed on a region has to do a cast:
Code Block |
---|
public void execute(FunctionContext context) {
RegionFunctionContext ctx = (RegionFunctionContext) context;
|
. This is not an intuitive API; the user has to know that there is such a thing as RegionFunctionContext. This can also lead to ClassCastExceptions if the user tries to cast the FunctionContext and it is not actually a REgionFunctionContextRegionFunctionContext
All onRegion function code basically has to start this way:
...
Instead, we should have a new RegionFunction interface that provides a RegionFunctionContext. FunctionService.onRegion should return a RegionExecution interface who's execute method should expect a RegionFunction.
See
Jira | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
...
RegionFunctionContext should provide the local data set
...
If you want to operate on the local data set for a function, you have to do this
...
- It's not clear that ctx.getDataSet does not return the local data.
- The local data set is not mockable for unit tests, because this static function call extracts it using internal APIs on concrete classes. This also prevents this sort of function from being tested in a pure unit test.
- The user has to know that PartitionRegionHelper exists; the API is not obvious
- The various methods on PartitionRegionHelper are confusing, specifically it's unclear to a user why they should use getLocalDataForContext instead of getLocalData
No way to get a spring data repository of the local data set of a function
This is not specifically an issue in the geode codebase, but it affects a lot of geode users.
Spring Gemfire provides an simplified function API. Spring Data Gemfire allows the user to generate a repository object to access a region. However, there is no way to generate a repository that accesses the local data for the function.
Here's an example
Code Block |
---|
public interface PostRepository extends GemfireRepository<Post, PostId> {
@Query("select * from /posts where id.person=$1")
public Collection<Post> findPosts(String personName);
}
public class Functions {
//This repository accesses the entire data for the region on all members.
@Autowired
PostRepository postRepository;
@GemfireFunction(HA=true)
public SentimentResult getSentimentDoesntWork(Region<PostId, Post> localPosts,
@Filter Set<String> personNames) throws Exception {
String personName = personNames.iterator().next();
//This works, because localPosts is bound to the local data for the region
Collection<Post> posts = localPosts.query("id.person='" + personName + "'");
//But I really want to use spring data repositories. Unfortunately,
//I can't get a repository here
PostRepository localDataRepository == ??
Collection<Post> posts = localDataRepository.findPosts(personName);
} |
FunctionService.onMembers relies on a singleton DistributedSystem
FunctionService.onMembers doesn't take a Cache or DistributedSystem as a parameter. That means we can't get rid of the singleton DistributedSystem, because FunctionService will always look up the singleton.
Having a static FunctionService.onXXX means users can't mock the functionService
To invoke a function, the user code needs to refer to the static FunctionService.onXXX methods ( see below). That means that code that contains these static calls can't be tested with a test that mocks the function service.
Code Block |
---|
FunctionService.onRegion(region)
.withArgs(context)
.withCollector(collector)
.execute(LuceneFunction.ID); |
It might be better to have a cache.getFunctionService
...
Instead, RegionFunctionContext should just provide a getLocalDataSet method.
The methods isHA, hasResult, optimizeForWrite should move from the Function interface to the Execution interface
isHA, hasResult, and optimizeForWrite are methods on Function that the user can override. However, that makes it harder to write lambda expressions for functions, because if you want to set one of these parameters you can now longer use a lambda.
Also the function service API provides a way to invoke functions using a string id. For example FunctionService.onServer().execute("MY_FUNCTION_ID"). If we remove these extra methods from the Function interface, this code will no longer need to look up the Function on the client and the Function class will not have to exist on the client anymore.
The Function interface should return a value
The function context has a ResultSender with methods like sendResult, lastResult, sendException, etc. Unfortunately, that makes the common case of a function that returns a single result unnecessarily verbose and error prone. For example, if no result is sent that can cause issues:
Code Block |
---|
public void execute(FunctionContext context) { if(someTest) { context.getResultSender().lastResult("Done"); } //Whoops I forgot to send a last result } |
Function code must exist on the client
The function service API provides a way to invoke functions by id, for example
Code Block |
---|
FunctionService.onServer().execute("MY_FUNCTION_ID") |
However, internally, we look up the function object itself from the function service in order to read the isHA, optimizeForWrite, and hasResult properties of the function. That means the code must exist on the client and must be registered with the FunctionService.
This is also an opportunity for the user to see an exception if the class on the client does not match the class on the server for these properties.
Lambdas and functions
Lambdas cannot be used for Geode functions, because there are multiple non abstract methods on function like hasResult, isHA, etc.
It would be nice to able to write short functions as lambda expressions, For example
Code Block |
---|
FunctionService.onMembers().execute( () -> System.out.println() ); |
However, that is not possible currently because Function has several non default methods.
We can also get rid of FunctionAdapter in favor of using default methods on Function.
See
Jira | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
Suggested API changes
Instead, the function interfaces should return a value. For the simple case where there is only one result, the user can just return the result. For cases with multiple streaming results, the user can look up the result sender. For cases with no results the user can just return null. The return value will be ignored anyway if the user invokes the function with hasResult==false.Instead of a single Function interface, there should be multiple interfaces for different types of functions. The function interface should return a value. For functions that don't return a value, they can return null.
Code Block |
---|
interface RegionFunction<T> { public T execute(RegionFunctionContext context) } interface Function<T> { public T execute(FunctionContext context) } |
RegionFunctionContext should have a method to the local data for the context
Code Block |
---|
RegionFunctionContext {
public Region getLocalData();
} |
FunctionContext should have a method to return the cache
Code Block |
---|
interface FunctionContext {
getCache();
} |
isHA, optimizeForWrite, and hasResult should be removed from Function and should be added to Execution instead
ResultCollector should have an addException method
Code Block |
---|
public interface ResultCollector<T,S> {
public void addException(DistributedMember member, Throwable t);
...
} |
Add Cache.getFunctionService and deprecate the static methods on FunctionService
This proposal might be a bit more controversial. The issue is that the current FunctionService leads people to write code that is harder to test, because they cannot mock the FunctionService if they are making static calls to FunctionService.onRegion. If FunctionService is something that could be provided by the cache, a user can mock FunctionService and assert that functions are invoked.
The other issue this fixes is that FunctionService has methods like onMembers(String ... groups) that internally look up a static cache. Besides making it hard for users to use Mocks with FunctionService code, this also ties us to having a singleton cache and distributed system. Since we're trying to get rid of singletons we should fix this API to not rely on an underlying singleton.
Stop Requiring the user to use the ResultCollector returned by the execute method.
The javadocs for execute say this
Code Block |
---|
/** @return ResultCollector to retrieve the results received. This is different
* object than the ResultCollector provided in
* {@link Execution#withCollector(ResultCollector)}. User has to use
* this reference to retrieve results.
*/ |
It's confusing to users that geode wraps their result collector in another collector that has different behavior, and it's unclear why the user has to call the returned result collector. We should stop wrapping the user's result collector, and instead provide helper classes like a BlockingResultCollector that waits for all results to arrive in getResults.
Below, we have to use the collector returned by FunctionService.execute, rather than just calling getResult on our own result collector. We run into problems otherwise, maybe because exceptions are not handed to the user supplied result collector? In any case, its unintuitive that this returns a different ResultCollector than the one supplied by the user.
Example:
Code Block |
---|
ResultCollector<TopEntriesCollector, TopEntries> rc = (ResultCollector<TopEntriesCollector, TopEntries>) FunctionService.onRegion(region)
.withArgs(context)
.withCollector(collector)
.execute(LuceneFunction.ID);
//You're not supposed to do this
TopEntries entries = collector.getResult()
//This is what you are supposed to do. rc != collector
TopEntries entries = rc.getResult();
|
Examples of API changes
Example of the improved API
Code Block |
---|
FunctionService functionService = cache.getFunctionService(); //RegionFunction with a single result ResultCollector<String, Collection<String>> rc = functionService.onRegion(region) .execute(ctx -> ctx.getLocalData().get("key")) Collection<String> values = rc.getResult() //RegionFunction without a result, that is optimized for write, but is not HA functionService.onRegion(region) .optimizeForWrite(true) .isHA(true) .hasResult(false) .execute(ctx -> ctx.getLocalData().put("key", "value)) //On member function that gets the cache from the context functionService.onMembers() .hasResult(false) .execute(ctx -> ctx.getCache().close()); //Function that streams results back using the result sender functionService.onRegion(region).execute( ctx -> { ResultSender<String> s = ctx.getResultSender(); for(String value : ctx.getRegion().values()) { s.addResult(value) } return null; } |