RFR 8168615: JShell API: jdk.jshell.spi should be a pluggable ServiceLoader SPI
Jan Lahoda
jan.lahoda at oracle.com
Mon Dec 12 15:20:06 UTC 2016
Seems Ok.
Minor nit: in JShell.java, there is (twice):
+ * Use, at most. one of these overloaded {@code
executionEngine} builder
+ * methods.
note the dot after "most" - should presumably be comma.
Jan
On 9.12.2016 03:50, Robert Field wrote:
> 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