Skip to content

HDDS-8858. Do not mix JSON output with hints.#4933

Merged
adoroszlai merged 1 commit intoapache:masterfrom
ashishkumar50:HDDS-8858
Jun 20, 2023
Merged

HDDS-8858. Do not mix JSON output with hints.#4933
adoroszlai merged 1 commit intoapache:masterfrom
ashishkumar50:HDDS-8858

Conversation

@ashishkumar50
Copy link
Contributor

What changes were proposed in this pull request?

We should avoid mixing JSON output with hints, which can lead to jq parse failure.
In this PR, In code where hints are printed with stdout is replaced by stderr.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-8858

How was this patch tested?

Verified in local cluster to see json/err redirection

$$ozone sh key list -l 1 o3://localhost:9862/vol11/ > a.txt
Listing first 1 entries of the result. Use --length (-l) to override max returned keys.

$$cat a.txt 
[ {
  "volumeName" : "vol11",
  "bucketName" : "bucket1",
  "name" : "key1",
  "dataSize" : 0,
  "creationTime" : "2023-06-19T11:04:46.584Z",
  "modificationTime" : "2023-06-19T11:04:46.640Z",
  "replicationConfig" : {
    "replicationFactor" : "THREE",
    "requiredNodes" : 3,
    "replicationType" : "RATIS"
  },
  "metadata" : { }
} ]

@ashishkumar50
Copy link
Contributor Author

@adoroszlai, Please help to review. Thanks

@sodonnel
Copy link
Contributor

As a general question - does the json output contain any hint that there are more records to retrieve?

Eg, should it have a startKey and hasMore = true / false so that a program can page down the results?

Also having a limit of 1 by default seems a bit strange - a default of 100 or something like that might be more useful.

Copy link
Contributor

@sodonnel sodonnel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this change makes sense - basically we are putting any help / warning text to stderr rather than stdout, which is generally the correct thing to do.

I think it would be nicer if the json was able to self describe better and give a hint that there are more records to consume. That way, a program could page through them more easily if needed, but that isn't really what this PR is for.

@ashishkumar50
Copy link
Contributor Author

@sodonnel, Thanks for the review.

does the json output contain any hint that there are more records to retrieve?
Json and hints are separated, stdout will contain only json that can be used by jq
Hints will be output using stderr so can be redirected separately or can be displayed on console.

Also having a limit of 1 by default seems a bit strange
---Default is 100, just I have less keys so I used -l 1 to override default value which is 100.

@adoroszlai
Copy link
Contributor

@sodonnel Agreed, such metadata in the JSON output would be nice, but that would mean changing the top-level list to a map, which is not backwards compatible. I hope we can make this change for 2.0.

having a limit of 1 by default seems a bit strange

Where do you see that default? The command in the description limits output to 1 item, but the default is 100 (in ListOptions).

@sodonnel
Copy link
Contributor

Ah, I just looked at the example output - didn't realize the default was not 1. Good to know its 100, as that is more sensible.

@adoroszlai adoroszlai merged commit 0652dea into apache:master Jun 20, 2023
@adoroszlai
Copy link
Contributor

Thanks @ashishkumar50 for the patch, @sodonnel for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants