Initial (not cluster safe) websocket change watcher implementation.#163
Initial (not cluster safe) websocket change watcher implementation.#163lvca merged 19 commits intoArcadeData:mainfrom
Conversation
lvca
left a comment
There was a problem hiding this comment.
Thanks, @tetious for your PR! Your code is very clean, even using the stuff available in the newer releases of Java, I'm surprised!
You can find my comments in the files as a review.
About using var we never did that because we were undecided if making everything compatible with Java8+, but we already have taken that decision, the minimum version is Java11, so using var is welcome and frankly suggested from now on to have more compact, less boilerplate code.
About the location, I think com.arcadedb.server.http.ws is the right place.
Good choice to let users subscribe to a single event. This is definitely more efficient and only the messages you're interested in are sent over the network.
server/src/main/java/com/arcadedb/server/http/WebSocketEventBus.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/arcadedb/server/http/WebSocketEventBus.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/arcadedb/server/http/WebSocketEventBus.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/arcadedb/server/http/WebSocketEventBus.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/arcadedb/server/http/WebSocketEventBus.java
Outdated
Show resolved
Hide resolved
Thanks very much for the excellent, detailed feedback! I'm a C#/Typescript guy mostly, and though I dabble widely, I haven't looked at Java since the Java 6 days. Things have improved so much since then! I still wish properties were a thing, though. 😄 I'll update the PR soon! |
|
@tetious thanks for all these changes. it's almost perfect. I've added a few comments. Thanks also for the initial test and the client class to make easier writing additional tests! |
Great to hear! I don't see your new comments, though. Maybe github ate them? I'm hoping to finish my todo list tonight and then possibly this will be ready. |
server/src/main/java/com/arcadedb/server/ws/WebSocketEventBus.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/arcadedb/server/ws/WebSocketEventBus.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/arcadedb/server/ws/WebSocketEventBus.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/arcadedb/server/ws/WebSocketEventBus.java
Outdated
Show resolved
Hide resolved
|
Sorry, I forgot to press "Review Changes" |
Remove database property. Use matchingSubscribers. Cleanups.
Cleanup getSubscriberSet.
Add two basic tests. Cleanups.
Properly shutdown the thread. Don't leak listeners on exceptions.
ec756e7 to
126b088
Compare
More cleanups. More tests.
|
I think this is nearly there. Still to do:
I'm running into a problem that I think might be some kind of race in the Listener logic. Most of the time, the first test I run in It appears as though It seems to be repeatable most of the time. Subsequent tests work fine, and reordering or running tests individually show it is always the first that fails. Thoughts? |
|
I was able to reproduce the NPE. Working on it. |
|
Fixed. I pushed the fix in this branch, now the test always passes. Thanks for discovering it. |
Rework WebSocketClientHelper to be less complicated. Cleanups.
Test connection loss.
|
I believe this is ready for a final review! I will write up some documentation tomorrow and submit a PR in the docs repo. |
lvca
left a comment
There was a problem hiding this comment.
Thanks, it's a high-quality contribution! I just wrote some comments/questions in the code, but for me, once git your answers and maybe little changes if needed, it's ready to be merged
server/src/main/java/com/arcadedb/server/http/ws/DatabaseEventWatcherThread.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/arcadedb/server/http/ws/WebSocketEventBus.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/arcadedb/server/http/ws/WebSocketEventBus.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/arcadedb/server/http/ws/WebSocketEventBus.java
Outdated
Show resolved
Hide resolved
Put subs in a nested map.
Be more flexible about message timeouts. Don't log handled errors.
|
I can't seem to repro the test failure locally, but I increased the timeout further and am now properly catching the null message case. Maybe this will work, but if not, maybe additional insight will appear. :) |
|
Trying to run the tests on my PC. |
|
Everything is fine on my PC. I guess we should extend the timeout, but 5s is already pretty long. If those tests will fail often we'll look into it. Thanks so much, @tetious, and congratulations on this big contribution! |
Bumps [at.yawk.lz4:lz4-java](https://github.com/yawkat/lz4-java) from 1.10.1 to 1.10.2. Release notes *Sourced from [at.yawk.lz4:lz4-java's releases](https://github.com/yawkat/lz4-java/releases).* > lz4-java v1.10.2 > ---------------- > > What's Changed > -------------- > > * Reproducible build by [`@yawkat`](https://github.com/yawkat) in [yawkat/lz4-java#15](https://redirect.github.com/yawkat/lz4-java/pull/15) > * Run tests for pull requests again by [`@Marcono1234`](https://github.com/Marcono1234) in [yawkat/lz4-java#17](https://redirect.github.com/yawkat/lz4-java/pull/17) > * Add `.git-versioned-pom.xml` to .gitignore by [`@Marcono1234`](https://github.com/Marcono1234) in [yawkat/lz4-java#16](https://redirect.github.com/yawkat/lz4-java/pull/16) > * Fix source code formatting by [`@Marcono1234`](https://github.com/Marcono1234) in [yawkat/lz4-java#18](https://redirect.github.com/yawkat/lz4-java/pull/18) > * Improve publish workflow by [`@Marcono1234`](https://github.com/Marcono1234) in [yawkat/lz4-java#19](https://redirect.github.com/yawkat/lz4-java/pull/19) > * Migrate to macOS 15 x86\_64 for release build by [`@Marcono1234`](https://github.com/Marcono1234) in [yawkat/lz4-java#21](https://redirect.github.com/yawkat/lz4-java/pull/21) > * Use gcc included in Windows image for release build by [`@Marcono1234`](https://github.com/Marcono1234) in [yawkat/lz4-java#22](https://redirect.github.com/yawkat/lz4-java/pull/22) > * Improve `LZ4FrameIOStreamTest` test by [`@Marcono1234`](https://github.com/Marcono1234) in [yawkat/lz4-java#23](https://redirect.github.com/yawkat/lz4-java/pull/23) > * Rename windows JNI lib to liblz4-java.dll by [`@HTHou`](https://github.com/HTHou) in [yawkat/lz4-java#25](https://redirect.github.com/yawkat/lz4-java/pull/25) > * Use bnd-maven-plugin to fix osgi manifest headers by [`@aptmac`](https://github.com/aptmac) in [yawkat/lz4-java#28](https://redirect.github.com/yawkat/lz4-java/pull/28) > > New Contributors > ---------------- > > * [`@HTHou`](https://github.com/HTHou) made their first contribution in [yawkat/lz4-java#25](https://redirect.github.com/yawkat/lz4-java/pull/25) > * [`@aptmac`](https://github.com/aptmac) made their first contribution in [yawkat/lz4-java#28](https://redirect.github.com/yawkat/lz4-java/pull/28) > > **Full Changelog**: <yawkat/lz4-java@v1.10.1...v1.10.2> Changelog *Sourced from [at.yawk.lz4:lz4-java's changelog](https://github.com/yawkat/lz4-java/blob/main/CHANGES.md).* > Change log > ========== > > 1.8.0 > ----- > > * Upgraded LZ4 to 1.9.3. Updated the JNI bindings. Minimum glibc version in GNU/Linux platforms: 2.17 on aarch64, 2.2.5 on amd64, 2.17 on ppc64le, 2.2 on s390x. > * Supported the JNI binding for Darwin aarch64. > * [#174](https://redirect.github.com/lz4/lz4-java/issues/174) > Fixed NullPointerException when reading a malformed input by LZ4FrameInputStream. > (Marco Berlot, Rei Odaira) > * [#169](https://redirect.github.com/lz4/lz4-java/issues/169) > Added information about requiring ant 1.10.2 or newer. > (guru prasad HB, Rei Odaira) > * [#167](https://redirect.github.com/lz4/lz4-java/issues/167) > Supported using LZ4SafeDecompressor in LZ4DecompressorWithLength. > (sahilpaudel-pe, Rei Odaira) > * [#163](https://redirect.github.com/lz4/lz4-java/issues/163) > Supported JNI binding in old CentOS 6 on amd64. > (dcapwell, Rei Odaira) > * [#162](https://redirect.github.com/lz4/lz4-java/issues/162) > Added copyright notices, required by Apache License, Version 2.0. > (Egor Nepomnyaschih, Rei Odaira) > * [#160](https://redirect.github.com/lz4/lz4-java/issues/160) > Added minimum glibc version to each release. > (patelh, Rei Odaira) > * [#146](https://redirect.github.com/lz4/lz4-java/issues/146) > Improved LZ4FrameInputStream to read InputStream lazily. > Instance creation of LZ4FrameInputStream became faster. > (Björn Michael, Rei Odaira) > * Improved the speed of the write methods of LZ4FrameOutputStream by > delaying calculating content checksum. > (Rei Odaira) > * Added a debug functionality to not delete a temporary JNI library > when either LZ4JAVA\_KEEP\_TEMP\_JNI\_LIB or lz4java.jnilib.temp.keep is > set to true (will be deleted in the next run). > (Rei Odaira) > * Enabled build with Java 13. The distribution is still > built with Java 7. (Rei Odaira) > * Revised the documentation. (Rei Odaira) ... (truncated) Commits * [`e3aa42c`](yawkat/lz4-java@e3aa42c) Use bnd-maven-plugin to fix osgi manifest headers ([#28](https://redirect.github.com/yawkat/lz4-java/issues/28)) * [`ef125a1`](yawkat/lz4-java@ef125a1) Rename net/jpountz/util/win32/amd64/liblz4-java.so to net/jpountz/util/window... * [`1dfbefd`](yawkat/lz4-java@1dfbefd) Improve `LZ4FrameIOStreamTest` test ([#23](https://redirect.github.com/yawkat/lz4-java/issues/23)) * [`de1e43e`](yawkat/lz4-java@de1e43e) Use gcc included in Windows image for release build ([#22](https://redirect.github.com/yawkat/lz4-java/issues/22)) * [`9ece12a`](yawkat/lz4-java@9ece12a) Migrate to macOS 15 x86\_64 for release build ([#21](https://redirect.github.com/yawkat/lz4-java/issues/21)) * [`37d698a`](yawkat/lz4-java@37d698a) Improve publish workflow ([#19](https://redirect.github.com/yawkat/lz4-java/issues/19)) * [`c491079`](yawkat/lz4-java@c491079) Fix source code formatting ([#18](https://redirect.github.com/yawkat/lz4-java/issues/18)) * [`eed5aa0`](yawkat/lz4-java@eed5aa0) Add `.git-versioned-pom.xml` to .gitignore ([#16](https://redirect.github.com/yawkat/lz4-java/issues/16)) * [`17f0e7a`](yawkat/lz4-java@17f0e7a) Run tests for pull requests again ([#17](https://redirect.github.com/yawkat/lz4-java/issues/17)) * [`37d6ca4`](yawkat/lz4-java@37d6ca4) Reproducible build ([#15](https://redirect.github.com/yawkat/lz4-java/issues/15)) * See full diff in [compare view](yawkat/lz4-java@v1.10.1...v1.10.2) [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- Dependabot commands and options You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
This is a WIP PR of the discussed (#149) web socket based change stream thingie. I have tested it lightly via Postman, so you can play with it now, but I'm sure there are plenty of ways to break it. 😄
You can connect like this:
ws://USERNAME:PASSWORD@localhost:2480/wsand send the following two messages:(for subscribe,
typeandchangeTypesare optional){"action":"subscribe", "database":"DATABASENAME", "type": "TYPENAME", "changeTypes": ["create", "update", "delete"]} {"action":"unsubscribe", "database":"DATABASENAME"}Still to come:
What does this PR do?
A WIP websocket based change stream. To be documented.
Motivation
See discussion #149.
Checklist
[x] I have run the build using
mvn clean packagecommand[x] My unit tests cover both failure and success scenarios