Skip to content
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

Merged
merged 2 commits into from
Mar 16, 2017
Merged

Conversation

begoldsm
Copy link
Contributor

@begoldsm begoldsm commented Mar 14, 2017

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

  • The title of the PR is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
  • Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
  • If applicable, the PR references the bug/issue that it fixes.
  • Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

  • I have read the contribution guidelines.
  • My spec meets the review criteria:
    • The spec conforms to the Swagger 2.0 specification.
    • Validation errors from the Linter extension for VS Code have all been fixed for this spec. (Note: for large, previously checked in specs, there will likely be many errors shown. Please contact our team so we can set a timeframe for fixing these errors if your PR is not going to address them).
    • The spec follows the patterns described in the Swagger good patterns document unless the service API makes this impossible.

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.
@sarangan12
Copy link
Member

I will review this PR and provide updates shortly

@begoldsm
Copy link
Contributor Author

@sarangan12 sounds good thanks!

@sarangan12
Copy link
Member

@begoldsm The swagger file is showing semantic validation error on the following paths:

/webhdfs/v1/{destinationPath}',
/webhdfs/v1/{msConcatDestinationPath}',
/webhdfs/v1/{listFilePath}',
/webhdfs/v1/{getContentSummaryFilePath}',
/webhdfs/v1/{getFilePath}',
/webhdfs/v1/{directFilePath}',
/webhdfs/v1/{setAclFilePath}',
/webhdfs/v1/{modifyAclFilePath}',
/webhdfs/v1/{removeAclFilePath}',
/webhdfs/v1/{defaultAclFilePath}',
/webhdfs/v1/{aclFilePath}',
/webhdfs/v1/{filePath}',
/webhdfs/v1/{renameFilePath}',
/webhdfs/v1/{setOwnerFilePath}',
/webhdfs/v1/{setPermissionFilePath}',

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?

@begoldsm
Copy link
Contributor Author

@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

@sarangan12
Copy link
Member

The following properties are not named, per the standards. The recommended name pattern is camelCase:
1. AclStatus
2. ContentSummary
3. FileStatus
4. FileStatuses
5. RemoteException

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

  1. #/paths/WebHdfsExt/{filePath}/put/operationId
  2. #/paths/webhdfs/v1/{path}/put/operationId
  3. #/paths/webhdfs/v1/{setAclFilePath}/put/operationId
  4. #/paths/webhdfs/v1/{modifyAclFilePath}/put/operationId
  5. #/paths/webhdfs/v1/{removeAclFilePath}/put/operationId
  6. #/paths/webhdfs/v1/{defaultAclFilePath}/put/operationId
  7. #/paths/webhdfs/v1/{aclFilePath}/put/operationId
  8. #/paths/webhdfs/v1/{renameFilePath}/put/operationId
  9. #/paths/webhdfs/v1/{setOwnerFilePath}/put/operationId
  10. #/paths/webhdfs/v1/{setPermissionFilePath}/put/operationId

The following are 'GET' operations in your swagger. The recommendation is to use the method name with 'Get'/method name must start with 'List'.

  1. #/paths/webhdfs/v1/{path}/get/operationId
  2. #/paths/webhdfs/v1/{directFilePath}/get/operationId

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.

@begoldsm
Copy link
Contributor Author

@sarangan12 responses to the validation logic:

  1. Camel Casing: won't fix, this is by design in WebHDFS, the payload json is in this format and will not be changed, because it must continue to be WebHDFS compliant
  2. where do you see text/json? It is not mentioned anywhere in this swagger spec.
  3. Won't fix, these are not creation methods. This is a filesystem API that must adhere to the naming conventions of WebHDFS. These are set operations that will add or replace, not create.
  4. Won't fix. The two get operations mentioned are not list operations. One opens and reads data from a file, the other validates if the caller has the specified access to a file or folder.
  5. Boolean values are returned for these (and must be to be compliant with webhdfs). I am ok with wrapping them into enums, but how would I bind "True" to one value and "False" to another one?
  6. Won't fix, we do not need to implement the operations API for a data plane swagger spec that does not ever hit ARM.

@sarangan12 sarangan12 merged commit 0ade654 into Azure:master Mar 16, 2017
@AutorestCI
Copy link

No modification for Ruby

@AutorestCI
Copy link

No modification for Python

@AutorestCI
Copy link

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