ACCUMULO-3948: Add Scanner classloader context #51
Conversation
|
|
||
| @Override | ||
| public void setContext(String context) { | ||
| this.context = context; |
There was a problem hiding this comment.
A Preconditions not null check would be nice here.
There was a problem hiding this comment.
I could, but it's not an error to pass null. The reference is initialized to null and is used successfully that way until it's set to something not null. Having said that, I don't really care one way or the other, just curious if we are forcing the user to use the api in a particular way.
There was a problem hiding this comment.
My thinking is there is no way to distinguish between a user accidentally passing null when they did not mean to and a user trying to clear the context. However if we do not accept null (users must call clearContext insead), then we can always detect the case where a user accidentally passes null and fail fast.
There was a problem hiding this comment.
However if we do not accept null (users must call clearContext insead)
IMO, we either need to have clear javadoc on setContext about what a null context means or just fail on null. I'd prefer the latter since there's an explicit clearContext method. That gives us more flexibility in the future to hide behind the interface.
|
Rat checks in core are passing for me. |
| @@ -242,4 +242,30 @@ | |||
| * @since 1.8.0 | |||
| */ | |||
| long getBatchTimeout(TimeUnit timeUnit); | |||
|
|
|||
| /** | |||
| * Sets the name of the classloader context on this scanner | |||
There was a problem hiding this comment.
Would be nice to add some text here pointing users to the Admin manual for more info. Don't know of a way to link with a URL, but we could still say go look at section X in the admin manual for more info.
There was a problem hiding this comment.
In addition to pointing users to admin manual, could also suggest looking at docs for relevant accumulo properties.
There was a problem hiding this comment.
And if the section number is added to the javadoc, who updates the javadoc in the future versions of the code when the manual changes? I would think that if you want to use the class loader context feature, then you have probably already read the section in the manual to understand how to configure it.
There was a problem hiding this comment.
And if the section number is added to the javadoc, who updates the javadoc in the future versions of the code when the manual changes?
Thats a good point. We could be vauge about it and just say see the user manual for more information. Also since its not a URL, we can not run tools to check for bad links.
I would think that if you want to use the class loader context feature, then you have probably already read the section in the manual to understand how to configure it.
I think its reasonable to assume that someone will start using 1.8 w/o ever reading the user manual and notice this new method in an IDE. When they see the new method in the IDE, currently it would not have much javadocs. However there are nice docs elsewhere. It would be nice to make the user aware of those docs and help them find them. Personally I think helping guide the user in this case is better than doing nothing because of the valid concern that info pointed to may move.
There was a problem hiding this comment.
Fair enough, I'll add a reference
There was a problem hiding this comment.
We could make the pointer a little more stable by using Accumulo version #. Like saying see section X in the 1.8.0 user manual.
There was a problem hiding this comment.
I was going to go with
/**
* Sets the name of the classloader context on this scanner. See
* the administration chapter of the user manual for details on
* how to configure and use classloader contexts.
*
* @param classLoaderContext
* name of the classloader context
* @throws NullPointerException
* if context is null
* @since 1.8.0
*/
void setClassLoaderContext(String classLoaderContext);
|
Its possible to set iterators on a Condition for a ConditionalMutation. So ConditionalWriterConfig should probably have a |
|
This is a code path that I am not familiar with. |
I can help with that if you like. I could do it as a follow in issue or a pull req to this pull req. |
|
I think I found it, running the build now. |
|
Can you point me to some tests that I need to modify? |
There is ConditionalWriterIT. Maybe could add a test where a conditional mutaton fails w/o a context and succeeds with it. |
| import org.junit.Before; | ||
| import org.junit.Test; | ||
|
|
||
| public class ScannerContextIT extends AccumuloClusterHarness { |
There was a problem hiding this comment.
A test to ensure there is no interference might be nice. Something like the following situation.
- Create scanner 1
- Create scanner 2
- Set context on scanner 1
- verify scanner 1 scans w/ context
- verify scanner 2 scans w/o any context.
Its not super important to have this test, it was just something I thought of. It think defends against odd leakage cases in the tablet server.
There was a problem hiding this comment.
I added this test in a commit today.
| @@ -316,6 +323,7 @@ public Options getOptions() { | |||
| "time before scan should fail if no data is returned. If no unit is given assumes seconds. Units d,h,m,s,and ms are supported. e.g. 30s or 100ms"); | |||
| outputFileOpt = new Option("o", "output", true, "local file to write the scan output to"); | |||
| sampleOpt = new Option(null, "sample", false, "Show sample"); | |||
| contextOpt = new Option("cc", "context", true, "name of the classloader context"); | |||
There was a problem hiding this comment.
Could add a test for this option ShellServerIT
|
Also, I added tickets 4054 and 4055 which are related to this. |
|
merged and pushed. |
No description provided.