Skip to content

perf: use normal feed for one-off changes #674

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.

Sign up for GitHub

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

Merged
ricellis merged 1 commit into from
Apr 22, 2025
Merged

perf: use normal feed for one-off changes #674

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/ChangesFollower.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ private synchronized Stream<ChangesResultItem> run(Mode mode) throws IllegalStat
// Construct the spliterator using the batch size as the per request limit
this.changesResultSpliterator.set(new ChangesResultSpliterator(
this.client,
ChangesOptionsHelper.cloneOptionsWithNewLimit(this.options, batchSize.get()),
ChangesOptionsHelper.cloneOptionsWithModeAndNewLimit(this.options, mode, batchSize.get()),
mode,
this.errorTolerance
));
Expand Down
67 changes: 31 additions & 36 deletions modules/cloudant/src/main/java/com/ibm/cloud/cloudant/features/ChangesOptionsHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,13 @@
import java.time.Duration;
import java.util.ArrayList;
import java.util.List;
import java.util.Optional;
import java.util.concurrent.TimeUnit;
import java.util.function.Consumer;
import java.util.stream.Collectors;
import com.ibm.cloud.cloudant.features.ChangesFollower.Mode;
import com.ibm.cloud.cloudant.v1.model.PostChangesOptions;
import com.ibm.cloud.cloudant.v1.model.PostChangesOptions.Builder;

class ChangesOptionsHelper {

Expand All @@ -36,51 +40,42 @@ private ChangesOptionsHelper() {
}

static PostChangesOptions cloneOptions(PostChangesOptions options) {
return cloneOptions(options, options.since(), options.limit());
return buildOptions(options, null);
}

static PostChangesOptions cloneOptionsWithNewLimit(PostChangesOptions options, Long limit) {
return cloneOptions(options, options.since(), limit);
static PostChangesOptions cloneOptionsWithModeAndNewLimit(PostChangesOptions options, Mode mode, Long limit) {
return buildOptions(options, b -> {
switch (mode) {
case FINITE:
b.feed(PostChangesOptions.Feed.NORMAL);
break;
case LISTEN:
b.feed(PostChangesOptions.Feed.LONGPOLL);
b.timeout(LONGPOLL_TIMEOUT);
break;
}
// Handle limit to avoid NPE during unboxing
if (limit != null) {
b.limit(limit);
}
});
}

static PostChangesOptions cloneOptionsWithNewSince(PostChangesOptions options, String since) {
return cloneOptions(options, since, options.limit());
return buildOptions(options, b -> {
b.since(since);
});
}

private static PostChangesOptions cloneOptions(PostChangesOptions options, String since, Long limit) {
// Now merge and set defaults
PostChangesOptions.Builder builder = new PostChangesOptions.Builder(options.db())
.attEncodingInfo(options.attEncodingInfo())
.attachments(options.attachments())
.conflicts(options.conflicts())
// no descending
.docIds(options.docIds())
.feed(PostChangesOptions.Feed.LONGPOLL)
.fields(options.fields())
.filter(options.filter())
// no heartbeat
.includeDocs(options.includeDocs())
// no lastEventId
// limit handled after
.selector(options.selector())
// seq interval handled after
.since(since)
.style(options.style())
.timeout(LONGPOLL_TIMEOUT)
.view(options.view());

// Handle options that might NPE during unboxing
if (limit != null) {
builder.limit(limit);
}
if (options.seqInterval() != null) {
builder.seqInterval(options.seqInterval());
}

private static PostChangesOptions buildOptions(PostChangesOptions originalOptions, Consumer<Builder> extraOpts) {
Builder builder = originalOptions.newBuilder();
Optional.ofNullable(extraOpts).ifPresent(
builderConsumer -> builderConsumer.accept(builder)
);
return builder.build();
}

private static String throwInvalidOptionsMessageWith(String suffix, List<String> invalidOptions) {
private static String throwInvalidOptionsMessageWith(List<String> invalidOptions) {

String errorMsgFormat = (invalidOptions.size() > 1) ? multipleOptionErrorFormat : singleOptionErrorFormat;
String errorMsg = String.format(errorMsgFormat,
Expand Down Expand Up @@ -112,7 +107,7 @@ static void validateOptions(PostChangesOptions options) {
invalidOptions.add(String.format("filter=%s", options.filter()));
}
if (invalidOptions.size() > 0) {
throwInvalidOptionsMessageWith(String.format(" when using %s.", ChangesFollower.class.getSimpleName()), invalidOptions);
throwInvalidOptionsMessageWith(invalidOptions);
Copy link
Contributor

@eiri eiri Apr 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No against it, but what's a rational behind this change?

Copy link
Member Author

@ricellis ricellis Apr 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was obsolete, I think the message was originally formatted here, but later it was adjusted and the suffix param of throwInvalidOptionsMessageWith was no longer used, but it wasn't removed. This just cleans up that oversight.

}
}
}
36 changes: 28 additions & 8 deletions modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/ChangesOptionsHelperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,12 @@

package com.ibm.cloud.cloudant.features;

import com.ibm.cloud.cloudant.features.ChangesFollower.Mode;
import com.ibm.cloud.cloudant.v1.model.PostChangesOptions;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Iterator;
import java.util.List;
import org.testng.Assert;
import org.testng.annotations.DataProvider;
import org.testng.annotations.Test;
Expand All @@ -29,27 +34,42 @@ Object[][] getValidOptionsDataProvider() {
return TestOptions.filteredValuesAsDataProvider(TestOptions::valid);
}

@DataProvider(name = "validOptionsBothModes")
Iterator<Object[]> getValidOptionsAndModesDataProvider() {
List<Object[]> options = new ArrayList<>();
for (Mode mode : Mode.values()) {
for (Object[] opts : getValidOptionsDataProvider()) {
// Add the valid opts
List<Object> modeOpts= new ArrayList<>(Arrays.asList(opts));
// Add the mode
modeOpts.add(mode);
options.add(modeOpts.toArray(new Object[modeOpts.size()]));
}
}
return options.iterator();
}

@DataProvider(name = "invalidOptions")
Object[][] getInvalidOptionsDataProvider() {
return TestOptions.filteredValuesAsDataProvider(o -> !o.valid());
}

@Test(dataProvider = "validOptions")
void testCloneOptions(String name, TestOptions opts) {
PostChangesOptions expected = opts.getExpectedOptions();
@Test(dataProvider = "validOptionsBothModes")
void testCloneOptions(String name, TestOptions opts, Mode mode) {
PostChangesOptions expected = opts.getExpectedOptions(mode);
Assert.assertNotNull(expected);
PostChangesOptions cloned = ChangesOptionsHelper.cloneOptions(expected);
// Can't use assertNotSame in testng 7.4 because https://github.com/cbeust/testng/issues/2486
Assert.assertFalse(cloned == expected, "The clone should not be the original options object.");
Assert.assertEquals(cloned, expected, "The clone should equal the original options object.");
}

@Test(dataProvider = "validOptions")
void testCloneOptionsWithNewLimit(String name, TestOptions opts) {
@Test(dataProvider = "validOptionsBothModes")
void testCloneOptionsWithNewLimit(String name, TestOptions opts, Mode mode) {
Long newLimit = 50L;
PostChangesOptions original = opts.getOptions();
PostChangesOptions expected = opts.getExpectedOptionsBuilder().limit(newLimit).build();
PostChangesOptions newLimitOpts = ChangesOptionsHelper.cloneOptionsWithNewLimit(original, newLimit);
PostChangesOptions expected = opts.getExpectedOptionsBuilder(mode).limit(newLimit).build();
PostChangesOptions newLimitOpts = ChangesOptionsHelper.cloneOptionsWithModeAndNewLimit(original, mode, newLimit);
// Can't use assertNotSame in testng 7.4 because https://github.com/cbeust/testng/issues/2486
Assert.assertFalse(newLimitOpts == original, "The clone should not be the original options object.");
Assert.assertNotEquals(newLimitOpts, original, "The clone should not equal the original options object.");
Expand All @@ -61,7 +81,7 @@ void testCloneOptionsWithNewLimit(String name, TestOptions opts) {
void testCloneOptionsWithNewSince(String name, TestOptions opts) {
String newSince = "9876-alotofcharactersthatarenotreallyrandom";
PostChangesOptions original = opts.getOptions();
PostChangesOptions expected = opts.getExpectedOptionsBuilder().since(newSince).build();
PostChangesOptions expected = opts.getExpectedOptionsBuilder(null).since(newSince).build();
PostChangesOptions newSinceOpts = ChangesOptionsHelper.cloneOptionsWithNewSince(original, newSince);
// Can't use assertNotSame in testng 7.4 because https://github.com/cbeust/testng/issues/2486
Assert.assertFalse(newSinceOpts == original, "The clone should not be the original options object.");
Expand Down
3 changes: 2 additions & 1 deletion .../cloudant/src/test/java/com/ibm/cloud/cloudant/features/ChangesResultSpliteratorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,9 @@
public class ChangesResultSpliteratorTest {

// A default set of options to run with
static final PostChangesOptions DEFAULT_OPTIONS = ChangesOptionsHelper.cloneOptionsWithNewLimit(
static final PostChangesOptions DEFAULT_OPTIONS = ChangesOptionsHelper.cloneOptionsWithModeAndNewLimit(
TestOptions.MINIMUM.getOptions(),
ChangesFollower.Mode.LISTEN,
ChangesFollower.BATCH_SIZE);

// A duration to use for testing error suppression
Expand Down
23 changes: 17 additions & 6 deletions modules/cloudant/src/test/java/com/ibm/cloud/cloudant/features/TestOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import java.util.function.Supplier;
import java.util.stream.Stream;
import com.ibm.cloud.cloudant.v1.model.PostChangesOptions;
import com.ibm.cloud.cloudant.features.ChangesFollower.Mode;
import com.ibm.cloud.cloudant.v1.model.GetDbUpdatesOptions.Feed;

/**
Expand Down Expand Up @@ -129,14 +130,24 @@ PostChangesOptions getOptions() {
}

// Adds in the implementation expected changes
PostChangesOptions.Builder getExpectedOptionsBuilder() {
return this.getBuilder()
.feed(Feed.LONGPOLL)
.timeout(ChangesOptionsHelper.LONGPOLL_TIMEOUT);
PostChangesOptions.Builder getExpectedOptionsBuilder(Mode mode) {
PostChangesOptions.Builder b = this.getBuilder();
if (mode != null) {
switch (mode) {
case FINITE:
b.feed(Feed.NORMAL);
break;
case LISTEN:
b.feed(Feed.LONGPOLL);
b.timeout(ChangesOptionsHelper.LONGPOLL_TIMEOUT);
break;
}
}
return b;
}

// Adds in the implementation expected changes
PostChangesOptions getExpectedOptions() {
return this.getExpectedOptionsBuilder().build();
PostChangesOptions getExpectedOptions(Mode mode) {
return this.getExpectedOptionsBuilder(mode).build();
}
}