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

avoid duplicating the Schema class in both the client and the server #66

Closed
junrao opened this issue Jan 27, 2015 · 4 comments
Closed
Assignees

Comments

@junrao
Copy link
Contributor

junrao commented Jan 27, 2015

The reason that the code is duplicated is that Schema on the server depends on Storage, which we don't want to drag into the client. We can probably refactor the code a bit to avoid the duplication.

@granders
Copy link
Contributor

RestUtils is also duplicated

@nehanarkhede nehanarkhede mentioned this issue Jan 29, 2015
@granders granders assigned granders and unassigned junrao Jan 30, 2015
@granders
Copy link
Contributor

granders commented Feb 2, 2015

@junrao @nehanarkhede
Neha and I discussed this a bit on Friday and came to the conclusion that there are actually two distinct functions of the Schema class

  • one is as a wrapper for the data that is returned to users on a GET schema request
  • the other is as a wrapper for data that is put into the kafka store

While the structure is currently identical, the class should not be shared for these two distinct purposes - so the preferred solution to this issue is to actually split into separate classes with less ambiguous names.

@ewencp
Copy link
Contributor

ewencp commented Feb 4, 2015

Adding some notes since I think discussion in other issues are assuming the fix for this issue is going to resolve a number of issues that have arisen as we've discussed how code is shared among the different modules. It increasingly sounds like this issue is now being treated as "Clean up the separation between the client and core modules".

  1. A lot of code sharing issues are also causing us to pull in Jersey dependencies into the client module. Pulling in Hibernate for validation is fine, but most other stuff like WebApplicationException don't make sense.
  2. RestUtils is duplicated, and also uses WebApplicationException to pass along errors. This needs to be replaced because, as added in a comment in Adding more specific error codes for HTTP 500 errors #98, it's just a hack that happens to make it easy to pass along responses from forwarded requests, but in practice ends up losing information (since it's throwing WebApplicationException rather than something more specific like RestException). Since RestUtils is used in client, it'll need some other exception which can pass along all the necessary info., Then, for uses in core where the response should be forwarded we'll need to convert it back.
  3. Other exceptions in client should not be extending WebApplicationException.
  4. There are some duplicated exceptions between client and core. It appears the ones in core are no longer being used and should probably be cleaned up, unless it turns out we need RestException versions for core and non-RestException versions for client.

@junrao
Copy link
Contributor Author

junrao commented Feb 6, 2015

Committed to master.

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

No branches or pull requests

3 participants