-
Notifications
You must be signed in to change notification settings - Fork 219
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
avro schema ui and validation #1107
avro schema ui and validation #1107
Conversation
@Big-Vi hi and thank you for this amazing contribution, we will take it to our side and keep you updated after reviewing your code and test it |
@Avitaltrifsik FYI |
@yanivbh1 Yes. I've worked on Go SDK already and code is in my local computer. If i get some reviews on this pull request, i can do some needed changes and can create pull request on Go SDK. |
@Big-Vi Hey I would love it if you can join our Discord channel :) |
Hi, @Avitaltrifsik I've joined the channel already. Thanks 🙂 |
@Big-Vi Awesome I DMed you :) |
6a87379
to
6f85542
Compare
6f85542
to
92dcdb2
Compare
ui_src/package.json
Outdated
@@ -41,6 +42,7 @@ | |||
"reaflow": "5.0.6", | |||
"recharts": "^2.1.16", | |||
"sass": "^1.49.0", | |||
"util": "^0.12.5", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are not using it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
value: 'avro', | ||
label: 'Avro (Coming soon)', | ||
value: 'Avro', | ||
label: 'Avro (New)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the (New)
value: 'avro', | ||
label: 'Avro (Coming soon)', | ||
value: 'Avro', | ||
label: 'Avro (New)', | ||
description: ( | ||
<span> | ||
The popular. Apache Avro™ is the leading serialization format for record data, and first choice for streaming data pipelines. It offers excellent schema |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a note saying that at the moment it is only available here to create schemas but they are not enforced yet on the SDK.
Also, make sure that in case of an Avro schema attached to a station not causing issues for connected clients or for clients who trying to produce/consume data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a note saying that at the moment it is only available here to create schemas but they are not enforced yet on the SDK. Also, make sure that in case of an Avro schema attached to a station not causing issues for connected clients or for clients who trying to produce/consume data
I can't find any better way to prevent connected clients from error out if the Avro schema is attached to the station. Can we update all the SDKs and merge all PRs at the same time? I'm all ears if you have any other suggestion.
I already worked on Go, JS, and Python SDKs. I can create a pull request today or tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So sure please open the PRs on the SDKs and we will merge this one only after it won't crash clients
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After merging the PRs of the SDKs please also update the readme file on this repo, you will find there a sdk features table
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So sure please open the PRs on the SDKs and we will merge this one only after it won't crash clients
Sweet as. Just to let you know I'm only familiar with the above mentioned languages. If somebody can work on the remaining SDKs(Java, .Net, and Rust), that would be great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After merging the PRs of the SDKs please also update the readme file on this repo, you will find there a sdk features table
Yep. Sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So sure please open the PRs on the SDKs and we will merge this one only after it won't crash clients
Sweet as. Just to let you know I'm only familiar with the above mentioned languages. If somebody can work on the remaining SDKs(Java, .Net, and Rust), that would be great.
Sure would you mind to open issues on those repositories?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. I'm on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So sure please open the PRs on the SDKs and we will merge this one only after it won't crash clients
Sweet as. Just to let you know I'm only familiar with the above mentioned languages. If somebody can work on the remaining SDKs(Java, .Net, and Rust), that would be great.
Sure would you mind to open issues on those repositories?
Issues opened:
superstreamlabs/memphis.java#42
superstreamlabs/memphis.net#105
For Rust, there's already an open issue. Not sure i need to open another one.
turulix/memphis-rust-community#15
if err != nil { | ||
return err | ||
} | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change to
return fmt.Errorf("your Avro file is invalid: %v", err.Error())
|
||
if schemaType == "protobuf" || schemaType == "json" || schemaType == "graphql" { | ||
if schemaType == "protobuf" || schemaType == "json" || schemaType == "graphql" || schemaType == "avro" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not related to here but we have the "GetFilterDetails" endpoint in which you should also return the Avro type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh. Yep. missed that one.
@Big-Vi I see your comments but not the code fixes, any chances you forgot to commit them? |
@idanasulinmemphis I like to know if you want me to replace "hamba/avro" with linkedin module before i do code fixes. |
No that is ok to leave it as is |
Great. I'll let you know once the fixes are done.
|
fixed ui filter removed util module better error handling
Hi @Big-Vi is it already done and awaiting my review? |
I fixed the code that you commented on. Good to review once again. |
7c5c724
to
236a5d7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Big-Vi approved, I'd like to thank you for this amazing contribution, it is much appreciated.
Before merging please resolve the conflict and that's it
Sweet as. Conflict resolved. |
@Big-Vi now it's ok, approved, and merged, will be released in the coming 1.1.2 version - next week |
1 - Added
https://github.com/apache/avro
for front-end validation.2 - Added node_module
util
to resolve polyfill issue - Stackoverflow3 - Added
https://github.com/hamba/avro
for backend schema validation.In progress #1061