Skip to content
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.

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

Multiple Filter Subjects Review #984

Merged
merged 24 commits into from
Sep 29, 2023
Merged

Conversation

scottf
Copy link
Contributor

@scottf scottf commented Sep 26, 2023

No description provided.

@@ -116,6 +115,7 @@ protected ConsumerConfiguration(ConsumerConfiguration cc) {
this.memStorage = cc.memStorage;
this.backoff = cc.backoff == null ? null : new ArrayList<>(cc.backoff);
this.metadata = cc.metadata == null ? null : new HashMap<>(cc.metadata);
this.filterSubjects = cc.filterSubjects == null ? null : new ArrayList<>(cc.filterSubjects);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was not released. I released that I need this to be null and not an empty list because it's the only way to determine if the user sets data in the list, which they could set empty, and this is used for comparison against a server version of the config during subscribe to ensure there are no mismatches.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite get it. You're setting this.filterSubjects to null only if cc.filterSubjects is null, so for empty list it would still be an empty list no? The only change here is that you make a copy of cc.filterSubjects.

Copy link
Contributor Author

@scottf scottf Sep 29, 2023

Choose a reason for hiding this comment

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

this.filterSubjects is final and must be set with something, even if it's null

The reason I copy it is because I always copy list input to constructors because people have reported bugs because they re-used a list or structure. Probably not necessary, just paranoid.

}
else {
filterSubjects = Collections.singletonList(tempFs);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved this down a bit in the code since it is more than a one liner

else if (filterSubjects.size() == 1) {
JsonUtils.addField(sb, FILTER_SUBJECT, filterSubjects.get(0));
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved down, no change

@@ -701,6 +712,9 @@ public Builder(ConsumerConfiguration cc) {
if (cc.metadata != null) {
this.metadata = new HashMap<>(cc.metadata);
}
if (cc.filterSubjects != null) {
this.filterSubjects = new ArrayList<>(cc.filterSubjects);
}
Copy link
Contributor Author

@scottf scottf Sep 28, 2023

Choose a reason for hiding this comment

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

always make a copy of lists because user's sometimes re-use and modify

createDefaultTestStream(jsm);
String stream = stream();
String subject = subject();
createMemoryStream(nc, stream, subject);
Copy link
Contributor Author

@scottf scottf Sep 28, 2023

Choose a reason for hiding this comment

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

Whenever I'm in tests now, I make them less generic, meaning I give unique values to things like stream and subject instead of using a fixed constant. There is no difference in test behavior, but this will allow me to re-use server instances in the future if necessary, as sometimes resources on build machines are low, and not having to start up a server every time would reduce failures based on inability to start servers. The change will allow allow more parallelization of tests.

@@ -293,7 +303,7 @@ public void testBasic() throws Exception {

@Test
public void testNoWait() throws Exception {
runInJsServer(nc -> {
runInJsServer(noPullWarnings(), nc -> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this reduces unnecessary output on tests.

@@ -545,55 +545,55 @@ public void testSemver() {
}

@Test
public void testListsAreEqual() {
public void testConsumerFilterSubjectsAreEquivalent() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed the name to match the method being tested, which changed it's name to clearer.

public static void runInJsServer(VersionCheck vc, ErrorListener el, InServerTest inServerTest) throws Exception {
runInServer(false, true, new Options.Builder().errorListener(el), vc, inServerTest);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

macro to make it easier to test only if a server version is appropriate. Useful during regression tests against older servers.

@@ -116,6 +115,7 @@ protected ConsumerConfiguration(ConsumerConfiguration cc) {
this.memStorage = cc.memStorage;
this.backoff = cc.backoff == null ? null : new ArrayList<>(cc.backoff);
this.metadata = cc.metadata == null ? null : new HashMap<>(cc.metadata);
this.filterSubjects = cc.filterSubjects == null ? null : new ArrayList<>(cc.filterSubjects);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite get it. You're setting this.filterSubjects to null only if cc.filterSubjects is null, so for empty list it would still be an empty list no? The only change here is that you make a copy of cc.filterSubjects.

@@ -44,6 +44,9 @@ public static String validateSubjectTerm(String subject, String label, boolean r
throw new IllegalArgumentException(label + " cannot end with '.'");
}

if (subject.contains(".>.")) {
int x = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we just throw error here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that was debug. I need to remove it.

Copy link
Contributor

@piotrpio piotrpio left a comment

Choose a reason for hiding this comment

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

LGTM!

@scottf scottf merged commit e3e5bad into main Sep 29, 2023
2 checks passed
@scottf scottf deleted the multiple-subject-filters-review branch September 29, 2023 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants