RFR 8168615: JShell API: jdk.jshell.spi should be a pluggable ServiceLoader SPI

[Jan Lahoda]

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.

>
>
>>
>> -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?

>
>>
>> -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?

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
>>>
>