Re: [SPDK] About dump/load (or export/edit/import) JSON file by SPDK

[Daniel Verkamp <daniel.verkamp at intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3962 bytes --]

On 02/02/2018 09:25 AM, Harris, James R wrote:
> *From: *SPDK <spdk-bounces(a)lists.01.org> on behalf of "Wodkowski, PawelX" <pawelx.wodkowski(a)intel.com>
> *Reply-To: *Storage Performance Development Kit <spdk(a)lists.01.org>
> *Date: *Friday, February 2, 2018 at 7:19 AM
> *To: *Storage Performance Development Kit <spdk(a)lists.01.org>
> *Subject: *Re: [SPDK] About dump/load (or export/edit/import) JSON file by SPDK
> 
> About your questions:
>> - the top layer of a JSON configuration file is a JSON object
> 
> I vote for this
> 
> I agree – making the JSON configuration file a JSON object seems like a good idea.
> 
> I would like to consider allowing comments + something like JSON-minify though.  https://github.com/getify/JSON.minify/tree/python
> 
> I’d love Daniel’s input on this – 50/50 chance he’ll either like the minify idea, or will flame me for suggesting it.

I agree that we definitely want to be able to have comments in the configuration file.

We already have support for (non-standard) JavaScript-style // and /* */ comments in our JSON parser; it just needs to be enabled with the SPDK_JSON_PARSE_FLAG_ALLOW_COMMENTS flag.

However, if we make the config file parser an external Python tool, we would probably need something like JSON-minify to remove the comments first (or a JSON parser that has non-standard comment support).

>> - the object has the following key-value pairs:
> 
>>  - key = "apptype", value = "name of application type",
> 
> I don’t see any value in adding this. If we decide to merge all apps into one capable of serving iSCSI, vhost,
> NVMeF what to do with this type of field. It will be dead anyway.
> 
> I agree with Pawel here.  I could see cases for testing where we may even describe the configuration using multiple JSON-RPC files.  For example, one configuration file to construct a bunch of block devices that are common across a bunch of different tests.  Then each test could provide a second configuration file that only provides specifics for the upper layer target (iSCSI, NVMe-oF, vhost, etc.)
> 
>>  - key = "jsonrpc", value = "2.0",
>>  - key = "sequence", value = an array of request objects which is made of ID, method, and params
>>  - Currently ID of JSON-RPC request is always 1.
>>  - Change it to the sequence number of each JSON-RPC request to call JSON-RPC requests in order.
> 
> User app might want to read many config files as well want to save config of each subsystem to different file.
> 
> I would vote against embedding “jsonrpc”, “id” etc in any JSON config file. Only data needed to fully restore
> application configuration should be needed in JSON config file.

Agreed, the jsonrpc and id fields shouldn't be necessary in the config file dump format.  "id" is only really useful for interactive RPC use where multiple requests are submitted at once; the "id" can be used to match up responses to requests, but in the config file case, we can submit the requests one at a time.

>> Do you think any general metadata should be included in the file or just sequence of requests is enough?
> 
> For other “metadata” reported by RPC calls there could be some additional call like “get_info”.
> 
> I think a sequence of requests is enough – plus ability to add comments in the JSON.
> 
> I think a new RPC that accepts a batch of other JSON-RPC objects would be challenging from an error handling perspective.  For example, what happens if the 4^th RPC out of a batch of 20 fails?  Sure – we could try to define a protocol for this but I think having the client just send one RPC at a time is sufficient for now.

I like the idea of having an external tool (possibly written in Python) that loads the JSON config file and sends RPC requests one at a time through the existing JSON-RPC interface; that keeps the SPDK library code modifications to a minimum.

Thanks,
-- Daniel