-
Notifications
You must be signed in to change notification settings - Fork 5.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Addressing WebHDFS team feedback about APIs #1034
Conversation
These are changes that are necessary in order to properly match the Front end contract, since our swagger specs are designed only for client construction, it is an ongoing process to bring them in line with the current state of the service. missed explicit required false. Filesystem updates Add leaseId Add FileSessionId Switch everything to not use MS* apis except for MSConcat. Address CR Feedback add optional "permission" to the CREATE calls Explicitly call out that APPEND is for serial appends. Address CR comments Include tooId for GET operations (files and acls) Add aclBit as a property that is returned in fileStatus fixed a copy/paste comment.
I will review this PR and provide updates shortly |
@sarangan12 sounds good thanks! |
@begoldsm The swagger file is showing semantic validation error on the following paths: /webhdfs/v1/{destinationPath}', All these paths look exactly the same (except for the parameter). So, could you please help me understand how the service is identifying which operation to perform? |
@sarangan12 this is by design. Swagger does not give us a good way of differentiating a method based on a query parameter. This is how webhdfs works, instead of having unique URI paths for unique operations, they use the op= query parameter. We enable this by setting the op query parameter to a constant value and having a "unique" path entry for each such operation (since they return/accept different parameters, requests and responses). Until there is a better way in swagger to differentiate paths by including query parameters, this was the recommendation given by @amarzavery |
The following properties are not named, per the standards. The recommended name pattern is camelCase: I see that your swagger file mentions 'application/json' & 'text/json' in your produces & consumes section. In the past, we have encountered services which mention both in their swagger file, but actually support only application/json. So, Could you please verify and confirm, if your service actually supports both 'application/json' & 'text/json'. The following are 'PUT' operations in your swagger. The recommendation is to use the method name with 'Create'.
The following are 'GET' operations in your swagger. The recommendation is to use the method name with 'Get'/method name must start with 'List'.
Booleans are not descriptive and make them hard to use. Instead use string enums with allowed set of values defined: 'FileOperationResult/boolean, AclStatus/stickyBit, FileStatusProperties/aclBit'. Operations API must be implemented for the service. |
@sarangan12 responses to the validation logic:
|
No modification for Ruby |
No modification for Python |
These are changes that are necessary in order to properly match the
Front end contract, since our swagger specs are designed only for client
construction, it is an ongoing process to bring them in line with the
current state of the service.
missed explicit required false.
Filesystem updates
Add leaseId
Add FileSessionId
Switch everything to not use MS* apis except for MSConcat.
Address CR Feedback
add optional "permission" to the CREATE calls
Explicitly call out that APPEND is for serial appends.
Address CR comments
Include tooId for GET operations (files and acls)
Add aclBit as a property that is returned in fileStatus
fixed a copy/paste comment.
This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.
PR information
api-version
in the path should match theapi-version
in the spec).Quality of Swagger