PERL-912/PERL-936 #166

phaylon · 2018-06-20T03:55:06Z

Note: This includes code for the fallback of startAtOperationTime, but disables it because:

When active tests fail with Resume of change stream was not possible, as the resume point may no longer be in the oplog
The spec test command event expectation data doesn't include, causing spec test failures.

Otherwise this PR includes the following changes:

Add Op::_ChangeStream as adapted version of Op::_Aggregate
Make MongoDB::ChangeStream use the new Op for command construction logic
Introduce startAtOperationTime (with disabled fallback, see above)
ChangeStream specification tests in t/changestream_spec.t
Add watch methods to ::Database and ::MongoClient
Add session as recognized option to the watch methods
Make ChangeStream tests also run on database and client, and test the session option

…r sessions

…failures

xdg

Looks like a great start! As a general point, I'd like to have the ChangeStreams.pm "_op_args" only be the cached _op_args fields from Collection (and the manually assembled equivalents in Database and MongoClient). All the watch API specific arguments and options should be in private variables. You'll see some notes to that effect in my comments.

xdg · 2018-06-20T05:16:12Z

lib/MongoDB/ChangeStream.pm

    ArrayOfHashRef
 );
 use Types::Standard qw(
+    Bool


Please use "Boolish" from MongoDB::_Types instead here and everywhere else instead, as it handles overloaded objects in a way the built-ins don't.

xdg · 2018-06-20T05:17:02Z

lib/MongoDB/ChangeStream.pm

 use Types::Standard qw(
+    Bool
    InstanceOf
+    Int


Ditto Intish

xdg · 2018-06-20T05:18:38Z

lib/MongoDB/ChangeStream.pm

+
+has start_at_operation_time => (
+    is => 'ro',
+    isa => Maybe[Int],


This is going to need to be a BSON::Timestamp, not Int, so that users can pass in the BSON::Timestamp they get back from session operation time.

It should also be private

xdg · 2018-06-20T05:20:33Z

lib/MongoDB/Collection.pm

+  C<startAtOperationTime>
 * C<maxAwaitTimeMS> - The maximum number of milliseconds for the server
  to wait before responding.
+* C<startAtOperationTime> - A timestamp specifying at what point in time


clarify that this should be a BSON::Timestamp

xdg · 2018-06-20T05:22:15Z

lib/MongoDB/Collection.pm


+    my $session = $self->_get_session_from_hashref( $options );
+
+    # boolify some options


Don't need these for change streams

xdg · 2018-06-20T13:58:43Z

lib/MongoDB/ChangeStream.pm

+    isa => MongoDBCollection,
 );

 has pipeline => (


On reflection, pipeline and full_document should be private, too. The only public method (other than the constructor) should be next.

xdg · 2018-06-20T14:00:48Z

lib/MongoDB/ChangeStream.pm

+has _op_args => (
    is => 'ro',
    isa => HashRef,
+    default => sub { {} },


This needs to be required without a default.

xdg · 2018-06-20T14:03:33Z

lib/MongoDB/ChangeStream.pm

+        pipeline => $self->pipeline,
+        all_changes_for_cluster => $self->all_changes_for_cluster,
+        changes_received => $self->_changes_received,
+        defined($self->start_at_operation_time)


I wonder if this should use _has_start_at_operation_time for consistency with resume token. Think again about it, but I'm not set on it if there is a good reason to check for definedness.

xdg · 2018-06-20T15:38:24Z

lib/MongoDB/Op/_ChangeStream.pm

+    my @pipeline = (
+        {'$changeStream' => {
+            (defined($start_op_time)
+                ? (startAtOperationTime => BSON::Timestamp->new(


If we make startAtOperationTime a BSON::Timestamp then this conversion isn't needed.

xdg · 2018-06-20T15:53:19Z

lib/MongoDB/Op/_ChangeStream.pm

+
+    # For explain, we give the whole response as fields have changed in
+    # different server versions
+    if ( $options->{explain} ) {


dont' need explain

xdg

One minor nit. Otherwise LGTM. Thanks!

xdg · 2018-06-21T16:04:55Z

lib/MongoDB/ChangeStream.pm

+    init_arg => 'start_at_operation_time',
+    predicate => '_has_start_at_operation_time',
+    coerce => sub {
+        ref($_[0]) ? $_[0] : BSON::Timestamp->new($_[0])


If you're wanting to allow seconds, I'd prefer to have this be BSON::Timestamp->new( seconds => $_[0]) rather than relying on BUILDARGS to fix up the legacy constructor format.

…argument name

xdg · 2018-06-22T20:46:27Z

Rebased and merged.

phaylon added 4 commits June 20, 2018 05:31

Create Op::_ChangeStream, add ->watch() to databases and clients

e56965a

Run changestream tests on databases and clients as well, add tests fo…

d052069

…r sessions

ChangeStream specification tests

a430849

Fixed startAtOperationTime fallback and disabled it due to spec test …

f909536

…failures

xdg suggested changes Jun 20, 2018

View reviewed changes

phaylon added 2 commits June 21, 2018 17:33

ChangeStream properties fixes and refactoring cleanups

866adcb

Don't send ChangeStream preset when resuming

b423da9

xdg suggested changes Jun 21, 2018

View reviewed changes

phaylon added 2 commits June 21, 2018 19:55

PERL-940, rework changestream resume logic

cad5139

move BSON::Timestamp constructor in coercion to use explicit seconds …

6c1b0c5

…argument name

xdg approved these changes Jun 22, 2018

View reviewed changes

xdg closed this Jun 22, 2018

TBSliver deleted the phaylon/PERL-912 branch June 13, 2019 14:52


		my $session = $self->_get_session_from_hashref( $options );

		# boolify some options

PERL-912/PERL-936 #166

PERL-912/PERL-936 #166

Uh oh!

Conversation

phaylon commented Jun 20, 2018

Uh oh!

xdg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xdg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xdg commented Jun 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants