Contributing valkeyJSON module #1
Conversation
Signed-off-by: Roshan Khatri <[email protected]>
Signed-off-by: Roshan Khatri <[email protected]>
Signed-off-by: Roshan Khatri <[email protected]>
|
Sorry，what's the blocker for JSON.MSET implementation? ^_^? |
There is no blocker. its WIP. That can be added in a separate PR. |
There was a problem hiding this comment.
There is no way that I can review all this. 27K in one commit. :D
There will need to be maintainer team for each module that can review each other's PRs. So, at least two maintainers would be useful. Is there anyone besides you @roshkhatri?
I'd say let's merge it! Later, we can talk about beta releases and stable releases. The TSC can oversee your release plans and the work on a high level, but I don't see that TSC members have enough time to be deeply involved in these modules.
Yes, @joehu21 will be the maintainer besides me.
Yes, once this is merged we can open issues and discuss/work on them. |
|
Mostly it looks like fine, agree to merge it. Great work. |
Signed-off-by: Roshan Khatri <[email protected]>
|
Ideally let's make issues for the comments here. The only thing I think we should fix before merging is getting the CI working. |
Signed-off-by: Roshan Khatri <[email protected]>
Adds ci.yml
Signed-off-by: Roshan Khatri <[email protected]>
|
I have added the CI workflow to run against valkey
|
There was a problem hiding this comment.
This does look overwhelming and I only reviewed haphazardly. I agree we should just merge this PR and polish it incrementally, including adding more test coverage.
Thanks @roshkhatri and @joehu21!
CMakeLists.txt
Outdated
| elseif("${ARCHITECTURE}" STREQUAL "aarch64") | ||
| message("Building JSON for aarch64") | ||
| else() | ||
| message(FATAL_ERROR "Unsupported architecture. JSON is only supported on x86_64 and aarch64.") |
There was a problem hiding this comment.
Yes, printing out ${ARCHITECTURE} will be helpful.
There was a problem hiding this comment.
looks like this is all about reusing the tests at https://github.com/valkey-io/valkey-bloom/tree/unstable/tests? or there is more?
Yes, it's for reusing the pytest infrastructure in valkey-bloom.
There was a problem hiding this comment.
hmm this dependency is a bit odd - does it make sense to pull the pytest infra into the Valkey repo instead? We are discussing migrating to python based integration test infra for Valkey too.
There was a problem hiding this comment.
Yes, I would suggest we should pull it out in the valkey repo instead. However, we can change the line later when we have it separately.
There was a problem hiding this comment.
I was going to suggest we write an RFC or something similar for the python testing framework as well before we get too far along building it organically.
There was a problem hiding this comment.
Why not move the testing framework to it's own repo to start with? It only needs to be pulled in for running the tests, not when just building the module.
I'd rather see people spend time on a README for the test framework than on an RFC. Or we can do both in parallel...?
There was a problem hiding this comment.
I'd rather see people spend time on a README for the test framework than on an RFC. Or we can do both in parallel...?
I just want us to have some long term plan for testing and not just build it up organically. The reason is that Amazon internally has a similar testing framework that is extremely complex because nobody really maintains it and just bolt on random features. There are many cases where it's possible to do the same way 2 or 3 different ways. I think the same is true of our TCL framework, just to a smaller scale.
So whether it be an RFC or just some direction in a new repo, I don't care which.
|
Before we merge, please just make sure everything has issues so we can track them. |
Signed-off-by: Roshan Khatri <[email protected]>
|
I have created relevant issues for the comments and resolved those conversations to track them better. I have also addressed some feedback for minor changes. Let me know if any more issues are required to be opens before we merge this PR. Happy Thanksgiving Everyone!! |
Signed-off-by: Roshan Khatri <[email protected]>
Happy Black Friday ! |
|
We have 6/6 TSC actually approved, so going ahead and merging it. |
This PR adds the implementation of valkeyJSON module based on the specifications detailed in the accepted RFC document: ValkeyJSON Specification.
The implementation does not include JSON.MSET implementation but I am working on it as we review this. I am hoping we could add the implementation post merging this PR.
@valkey-io/core-team