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

ISSUE-1240: Handle schema branches for all sources rather than just Kafka #1243

Closed
wants to merge 1 commit into from

Conversation

priyank5485
Copy link
Contributor

@priyank5485 priyank5485 commented Mar 16, 2018

No description provided.

@priyank5485
Copy link
Contributor Author

@shahsank3t @joylyn

While handling ui changes in https://github.com/hortonworks/streamline/pull/1204/files for backend changes in #1200 it seems that we missed a few things.

Can we fix the below?

  1. It seems that default value of MASTER for schema branch is hardcoded for field name. Quoting an example for source at https://github.com/hortonworks/streamline/pull/1204/files#diff-700814ca73bcf79d2404b6826901cebfR115 Ditto for sink. Can we change the logic in that if clause such that we use hint instead of fieldName? So, if there are any fields for that source/sink that have a hint of schemaBranch and if they dont have a value then we assign the default value of master.
  2. The logic for getting all branches for a schema also seems to be hard coded just for Kafka. https://github.com/hortonworks/streamline/pull/1204/files#diff-700814ca73bcf79d2404b6826901cebfR145 ditto for sink. That is making all other sources fail. I dont see a call being made to fetch branches for other sources like hdfs, eventhub, etc after applying json file changes in this PR. Can we change that logic instead to use the hints schemaBranch and dependsOn- to make it generic and work for all ?

In general if you guys come across any UI api calls that are hardcoded using field names, please change it to use hints as well, unless its a special case.

@arunmahadevan The changes I have pushed in this PR are necessary and will not break anything. However they are not sufficient and we need UI fixes as mentioned above as well to fix this issue.

@sameer79
Copy link
Contributor

Raised PR : #1244

Copy link
Contributor

@shahsank3t shahsank3t left a comment

Choose a reason for hiding this comment

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

+1
Thanks for the details @priyank5485
Your changes look good. UI changes are done by @sameer79 and have already raised another PR.

…afka

Change-Id: Iade05199c2c38ce9781344b64d64344c65bc2990
@priyank5485
Copy link
Contributor Author

@shahsank3t @sameer79 Thanks for the UI fixes. @arunmahadevan I have tested that originally reported issue for ui sending request for undefined schema is fixed. Tested with hdfs and eventhubs. Please review and merge when you get a chance along with the other PR raised by @sameer79

@arunmahadevan arunmahadevan changed the title ISSUE-1240: Handle schema branches for all sources rather than just K… ISSUE-1240: Handle schema branches for all sources rather than just Kafka Mar 19, 2018
@arunmahadevan
Copy link
Contributor

+1

arunmahadevan pushed a commit that referenced this pull request Mar 19, 2018
shahsank3t  priyank5485 arunmahadevan

Depends on: #1243
Address all comment mention in PR #1243.

Handle schema branches for all sources and sinks.

priyank5485  please verify at your end as well, Thanks in advanced.

Author: sameer79 <findsameershaikh@yahoo.co.in>

Closes #1244 from sameer79/ISSUE-1240
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