You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucenenet.apache.org by NightOwl888 <gi...@git.apache.org> on 2017/05/16 17:12:35 UTC
[GitHub] lucenenet pull request #207: API: Added ReferenceManager.AcquireContext()
GitHub user NightOwl888 opened a pull request:
https://github.com/apache/lucenenet/pull/207
API: Added ReferenceManager<G>.AcquireContext()
Opening this as a pull request so the team can discuss this approach vs the previous `ExecuteSearch` method (that was inadvertently removed from the API).
## AcquireContext Example
```c#
var searcherManager = new SearcherManager(indexWriter, true, null);
using (var context = searcherManager.AcquireContext())
{
IndexSearcher searcher = context.Reference;
var topDocs = searcher.Search(query, 10);
// use results...
}
```
### Pros
1. Eliminates the try-finally block.
2. Doesn't swallow exceptions by default.
3. Implicitly cleans up.
4. Since the extension method is on `ReferenceManager<G>`, all of its subclasses automatically gain this functionality.
### Cons
1. If the user forgets the using block, cleanup is not automatic.
## ExecuteSearch Example
```c#
var searcherManager = new SearcherManager(indexWriter, true, null);
searcherManager.ExecuteSearch(searcher =>
{
var topDocs = searcher.Search(query, 10);
// use results...
}, exception => { Console.WriteLine(exception.ToString()); });
```
### Pros
1. Eliminates the try-finally block.
2. Implicitly cleans up.
3. `ExecuteSearch` name makes it easier to find on the API.
4. The user doesn't need to remember any special syntax to clean up.
### Cons
1. Swallows exceptions by default. To make the exception bubble, you have to re-throw it.
2. API only applies to SearcherManager, so similar code needs to be repeated for other `ReferenceManager<G>` subclasses.
### Discussion
Although the `ExecuteSearch` method is well named and thus easier to find on the API, it has the disadvantage of swallowing exceptions by default. Exceptions do not automatically bubble - you have to re-throw the exception (in which case performance suffers and you lose the stack trace). Its main advantage is that there is no way for the user to accidentally forget to clean up.
The pattern seems to lend itself better to a using block. If all code examples are shown with a using block the user is not likely to forget it. However, admittedly, `AcquireContext` and `context.Reference` are probably not the most intuitive names (I was torn between `Context` or `Holder` for the name, but there might be a better choice than either of those). `context.Reference` was chosen because `ReferenceContext` is the name of the generic class that holds the reference and it applies to all subclasses of `ReferenceManager<G>`. Of course, the original API was named `Acquire()`, so it seems logical to use `AcquireContext()` or `AcquireHolder()` or something along those lines. Ideas welcome.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/NightOwl888/lucenenet api-reference-context
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/lucenenet/pull/207.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #207
----
commit dc9bbc056f678ee2cdd63b0a877c1ff75c37f14a
Author: Shad Storhaug <sh...@shadstorhaug.com>
Date: 2017-05-11T06:44:46Z
API: Added ReferenceManager<G>.AcquireContext(), which is similar to ReferenceManager<G>.Acquire() but can be used in a using block to implicitly dereference instead of having to do it explicitly in a finally block.
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet issue #207: API: Added ReferenceManager.AcquireContext()
Posted by NightOwl888 <gi...@git.apache.org>.
Github user NightOwl888 commented on the issue:
https://github.com/apache/lucenenet/pull/207
Looks nice. Although, I tend to favor the "all methods are verbs" and "all properties are nouns or adjectives" methodology. For example, I would expect a method named `Buffer()` to buffer something, not return a buffer. In this case that argument is a bit weaker, but it still feels like `GetContext()` would be more clear than `Context()`.
I primarily named it `AcquireContext()` because it is essentially the same functionality as the existing `Acquire()` method (from Java), so the similar names seem natural. But `GetContext()` is also clear (and more concise).
Hmm, that wasn't what I meant by "other subclasses". Actually, I made the constructor of `ReferenceContext` internal so it is effectively sealed to the outside world. What I was referring to is the fact that SearcherManger is just one of a few (with many more possible) subclasses of `ReferenceManager<G>`.
And since `ReferenceManager<G>` is generic, the `Reference` property is also generic and the cast is not necessary - its type is specific to the subclass of `ReferenceManager<G>`.
However, we can't very well name the property `Searcher` (so we have `context.Searcher`) because it wouldn't be generic enough for the other subclasses of `ReferenceManager<G>`. Well, we could by making extension methods and a custom "holder" type for each subclass of `ReferenceManager<G>`. But the disadvantage there is repetitive similar code. Maybe it would be worth it for such an integral part of the API, though.
The purpose of the using block here isn't to dispose anything really - it is just to help ensure the variable isn't utilized after its reference is no longer tracked. The context is just a "holder" for the reference to implement `Dispose()` on, which tells the parent `ReferenceManager<G>` class that we are done using the reference. You can see an example of how it looks with the Java functions [here](https://github.com/synhershko/LuceneNetDemo/blob/master/LuceneNetDemo/GitHubIndex.cs#L157-L184) with a more verbose finally block. Same thing, but shaves a few lines of code off of the block. You don't gain much if you need to catch an exception, though because you still need to put a try catch inside of the using block.
The using block does do one nice thing. Since it is a block, the variable goes out of scope when it is done.
```c#
using (var context = searcherManager.AcquireContext)
{
var foo = context.Reference;
foo.Search();
} // foo goes out of scope
```
The only issue is that it is not 100% obvious that this is a problem:
```c#
Searcher foo;
using (var context = searcherManager.AcquireContext)
{
foo = context.Reference;
} // foo doesn't go out of scope - BAD
foo.Search(); // Not allowed here
```
But this just seems to be a scenario where there is no real way to force the end user to follow the rules. You just have to document the right way and hope they follow the documentation.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet issue #207: API: Added ReferenceManager.AcquireContext()
Posted by geobmx540 <gi...@git.apache.org>.
Github user geobmx540 commented on the issue:
https://github.com/apache/lucenenet/pull/207
Ah, ok I misunderstood your use case regarding other subclasses. I agree, the amount of repetition in the code for that is probably unwieldy.
No argument with `GetContext()`, even though it doesn't match java, it matches .net style (i.e. `GetEnumerator()`
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet pull request #207: API: Added ReferenceManager.AcquireContext()
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/lucenenet/pull/207
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet issue #207: API: Added ReferenceManager.AcquireContext()
Posted by NightOwl888 <gi...@git.apache.org>.
Github user NightOwl888 commented on the issue:
https://github.com/apache/lucenenet/pull/207
What about naming? Do you think it would be more intuitive to call it `AcquireReference()` and make the class named `Reference` instead of `ReferenceContext`? Seems like that would be a bit more intuitive from a user point of view.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet issue #207: API: Added ReferenceManager.AcquireContext()
Posted by geobmx540 <gi...@git.apache.org>.
Github user geobmx540 commented on the issue:
https://github.com/apache/lucenenet/pull/207
The first approach is much more intuitive and inline with the rest of the C# ecosystem.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet issue #207: API: Added ReferenceManager.AcquireContext()
Posted by geobmx540 <gi...@git.apache.org>.
Github user geobmx540 commented on the issue:
https://github.com/apache/lucenenet/pull/207
Is our reference only an `IndexSearcher` and it's descendants? I'm partial to the `Context` language - I'm using EF as my implementation reference because I think it's widely known, accepted and pretty well architected. `DbContext` is well known, just as perhaps `LuceneContext` could be very intuitive (as the class) with the method 'Context()'.
We could also overload `ReferenceContext.cs` (or `LuceneContext`) to have pass through methods directly to the context/reference IndexSearch to avoid the extra method call to get the Reference. This would require an additional constraint on `T`
```
using (var context = searcherManager.Context())
{
// General use case
var topDocs = context.Search(query, 10);
// Cast if you're trying to reach a descendant class type with additional methods / properties
BetterIndexSearcher searcher = (BetterIndexSearcher)context.Context;
var betterTopDocs = searcher.BetterSearch(query,10);
}
```
or alternatively,
using (var context = (BetterIndexSearcher)searcherManager.Context())
{
var topDocs = context.BetterSearch(query, 10);
}
```
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---