RFR 8168615: JShell API: jdk.jshell.spi should be a pluggable ServiceLoader SPI
Robert Field
robert.field at oracle.com
Fri Dec 9 02:50:43 UTC 2016
Updated version (v2) --
JShell docs:
http://cr.openjdk.java.net/~rfield/8168615v2.jshellAPI/
Specdiff:
http://cr.openjdk.java.net/~rfield/8168615v2.specdiff/overview-summary.html
Webrev:
http://cr.openjdk.java.net/~rfield/8168615v2.webrev/
Responses and references to the above in-line...
On 12/08/16 13:02, Jan Lahoda wrote:
> On 7.12.2016 06:08, Robert Field wrote:
>> Thanks for the review, Jan.
>>
>> See responses inline.
>>
>> On 12/06/16 10:14, Jan Lahoda wrote:
>>> A few comments:
>>> -the -X help will (in my understanding) produce this:
>>> +\ --execution <spec> Specify an alternate
>>> execution engine.\n\
>>> +\ Where <spec> is an
>>> ExecutionControldProvider spec.\n\
>>> +\ See jdk.jshell.spi\n\
>>>
>>> As this is a user-level help (although a very experienced user,
>>> presumably), it would be better I think to provide some details - e.g.
>>> the grammar for the spec; known provider; and ideally, parameters of
>>> the known providers
>>
>> OK, so better doc references are in order. I think including the
>> grammar and explanatory information is probably too bulky for
>> command-line help. But I've clarified this help as follows (the
>> formatting is right if fix-width, but formatted text cannot be sent to
>> this list):
>>
>> % jshell -X
>> --add-exports <module>/<package> Export specified
>> module-private package to snippets
>> --execution <spec> Specify an alternate
>> execution engine.
>> Where <spec> is an
>> ExecutionControl spec.
>> See the documentation
>> of the package
>> jdk.jshell.spi for
>> the syntax of the spec
>>
>> These options are non-standard and subject to change without
>> notice.
>>
>> Where the jdk.jshell.spi javadocs are now:
>>
>> http://cr.openjdk.java.net/~rfield/8168615v1.jshellAPI/jdk/jshell/spi/package-summary.html
>>
>>
>>
>> It is OK that they would need to reference the SPI docs because if they
>> implemented an execution engine they would have implemented these
>> interfaces, If they are just tweaking parameters (and this isn't
>> something we want a typical user to do) they would need to be familiar
>> with the parameters, which are described in these docs.
>
> Still not sure if it is polite to not provide enough information for
> the user to construct the option, but OK.
The SPI is and documents the interface, so it doesn't seem an ideal
place to document the set of current implementations -- though, by
example, it demonstrates specs for all the predefined providers and
almost all of the their parameters.
The only reason this is here or interesting is if they are defining
their own execution engine, in which case they need to be familiar.
In fact, I'm negatively inclined to have the average users throwing
random spec strings at it and submitting bugs when they get failures.
>
>>
>>
>>>
>>> -I would reorganize the rules in the spec grammar to go from the top
>>> down:
>>> + * spec := name : params
>>> + * | name
>>> + * name := id
>>> + * params := param , params
>>> + * | param
>>> + * |
>>> + * param := id ( value )
>>>
>>> (might be also good to say what is "id")
>>
>> Both good ideas implemented in the above.
>>
>>>
>>> -ExecutionControlProvider.logMessages seems weird. Is it used outside
>>> of tests?
>>
>> Not currently, but we really need logging of failures and failover to
>> track-down rare intermittent issues. Adding logging is out of scope for
>> 9, but this gives a needed hook.
>
> So, why not use a logging framework (e.g. System.getLogger) for logging?
Fair enough. I've switched to the logging framework and removed
ExecutionControlProvider.logMessages.
http://cr.openjdk.java.net/~rfield/8168615v2.jshellAPI/jdk/jshell/spi/ExecutionControlProvider.html
http://cr.openjdk.java.net/~rfield/8168615v2.webrev/src/jdk.jshell/share/classes/jdk/jshell/execution/FailOverExecutionControlProvider.java.html
http://cr.openjdk.java.net/~rfield/8168615v2.webrev/test/jdk/jshell/FailOverDirectExecutionControlTest.java.html
>
>>
>>>
>>> -after this patch, both the ExecutionControls and their Providers are
>>> public - is that desirable? Is there a usecase for the client to
>>> call/instantiate the Providers directly without going through the
>>> ServiceLoader?
>>
>> The ExecutionControls were public before. jdk.jshell.execution contains
>> the default execution engines, but it is exposed because it is a library
>> of classes for implementing ExecutionControl (and now
>> ExecutionControlProvider).
>
> What is the usecase for having the LocalExecutionControlProvider,
> JdiExecutionControlProvider and FailOverExecutionControlProvider public?
jdk.jshell.execution is for public implementations of the jdk.jshell.spi
interfaces, and those interfaces now include ExecutionControlProvider,
basically replacing ExecutionControl.Generator.
JdiDefaultExecutionControl or JdiExecutionControl are designed to
function as both standard implementation and subclassable code for
constructing new execution engines. The provider is the face of this
body, it would seem strange to expose the body and not the face. For
example, JdiExecutionControlProvider, were JdiDefaultExecutionControl
subclassed, it would need a provider -- the obvious choice would be to
subclass JdiExecutionControlProvider changng out the ExecutionControl
implementation and retaining the rest of the provider functionality.
The subclassing is far less compelling for LocalExecutionControlProvider
since both its methods would need to be overridden.
For each of them, this is where the parameters are specified -- so they
need to be in public documentation. Given that though,
defaultParameters() should be overriden in LocalExecutionControlProvider
to specify there are no parameters.
http://cr.openjdk.java.net/~rfield/8168615v2.jshellAPI/jdk/jshell/execution/LocalExecutionControlProvider.html#defaultParameters--
-Robert
>
> Jan
>
>>
>> The SPI and the jdk.jshell.execution library were created because of a
>> request from an IDE developer.
>>
>> OK, here is the new version. No executable code was changed, but I've
>> spruced-up the documentation, including what is mentioned above.
>>
>> The docs:
>>
>> http://cr.openjdk.java.net/~rfield/8168615v1.jshellAPI/
>>
>> Specdiff:
>>
>> http://cr.openjdk.java.net/~rfield/8168615v1.specdiff/overview-summary.html
>>
>>
>> New webrev:
>>
>> http://cr.openjdk.java.net/~rfield/8168615v1.webrev/
>>
>> -Robert
>>
>>>
>>> Jan
>>>
>>> On 2.12.2016 08:48, Robert Field wrote:
>>>> Please review...
>>>>
>>>> Bug:
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8168615
>>>>
>>>>
>>>> Webrev:
>>>>
>>>> http://cr.openjdk.java.net/~rfield/8168615v0.webrev/
>>>>
>>>> Thanks,
>>>> Robert
>>>>
>>
More information about the kulla-dev
mailing list