add support notification from database#17
add support notification from database#17andrewst wants to merge 3 commits intostablekernel:masterfrom
Conversation
|
I think bad test "Notification many channel", any ideas? |
Codecov Report
@@ Coverage Diff @@
## master #17 +/- ##
==========================================
- Coverage 90.28% 90.19% -0.09%
==========================================
Files 11 11
Lines 947 969 +22
==========================================
+ Hits 855 874 +19
- Misses 92 95 +3
Continue to review full report at Codecov.
|
|
May be better moving process NotificationResponse into PostgreSQLConnection._readData? |
itsjoeconway
left a comment
itsjoeconway
left a comment
There was a problem hiding this comment.
GREAT tests! Few minor things to move around in the implementation.
| _connectionState.connection = this; | ||
| } | ||
|
|
||
| final StreamController<Notification> _notifications = new StreamController<Notification>.broadcast(); |
There was a problem hiding this comment.
Close this resource in PostgreSQLConnection.close(); make sure the future returned by PostgreSQLConnection.close() doesn't complete until StreamController.close()'s future completes
| @@ -12,6 +12,17 @@ abstract class _PostgreSQLConnectionState { | |||
| } | |||
|
|
|||
| _PostgreSQLConnectionState onMessage(ServerMessage message) { | |||
There was a problem hiding this comment.
Since notifications don't have quite the same lifecycle or need for the finite state machine, it'd be better to make this if (message is NotificationResponseMessage) check in connection._readData and avoid the FSM altogether. There is already logic in connection._readData to separate out ErrorResponseMessages, so it'll fit right in.
There was a problem hiding this comment.
Also, since (and if I'm wrong correct me) if NotificationResponseMessages come out of band - i.e. a Query does not trigger them, they are sent to to the user without request - we should wrap their processing in a try-catch block and emit an error on the broadcast stream if something within that processing fails.
There was a problem hiding this comment.
I don't understand how better make this, please hint me.
There was a problem hiding this comment.
Yeah, I'm not sure if it makes sense - ignore this one
lib/src/server_messages.dart
Outdated
| void readBytes(Uint8List bytes) { | ||
| var view = new ByteData.view(bytes.buffer, bytes.offsetInBytes); | ||
| processId = view.getUint32(0); | ||
| channel = UTF8.decode(bytes.sublist(4, bytes.indexOf(0, 4))); |
There was a problem hiding this comment.
You can avoid a copy (bytes.sublist) on this line and the next by using UTF8.decoder.startChunkedConversion and piping it bytes from the view.
There was a problem hiding this comment.
Bah, this is my fault - I think your earlier implementation was just fine. If there really is a need to optimize this, there are a few other places to do it too, and that can wait. Can you revert back to what you had here? And then making the processID public on the connection, I think we're all set.
There was a problem hiding this comment.
ok, I revert this, and create new clean PR.
| } | ||
|
|
||
| class Notification { | ||
| final int processId; |
There was a problem hiding this comment.
Capitalize the trailing d, e.g. ID to stay consistent with the Connection._processID
There was a problem hiding this comment.
Also, is it possible to have some way of having the connection know if the notification was triggered by that connection? Would allow listeners to say 'Oh, I sent this, I can ignore it' but would also allow listeners to structure their app so they can listen for their own notifications, too.
There was a problem hiding this comment.
I don't find in documentation postgresql nothing this case.
There was a problem hiding this comment.
I believe that when a connection is made a process ID is assigned to it (this should be stored in Connection._processID). When NOTIFY is delivered, the process ID is included in the delivery to each subscriber that matches the process ID of the connection that triggered the NOTIFY. An interested subscriber may want to be able to filter out NOTIFYs that they themselves sent.
There was a problem hiding this comment.
May be property PostgreSQLConnection._pricessID make public?
There was a problem hiding this comment.
Yeah, it can get switched to public
3 participants
No description provided.