Multiple Filter Subjects Review #984
Conversation
| @@ -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); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); | ||
| } |
There was a problem hiding this comment.
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)); | ||
| } | ||
| } |
There was a problem hiding this comment.
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); | |||
| } | |||
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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 -> { | |||
There was a problem hiding this comment.
this reduces unnecessary output on tests.
| @@ -545,55 +545,55 @@ public void testSemver() { | |||
| } | |||
|
|
|||
| @Test | |||
| public void testListsAreEqual() { | |||
| public void testConsumerFilterSubjectsAreEquivalent() { | |||
There was a problem hiding this comment.
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); | ||
| } | ||
|
|
There was a problem hiding this comment.
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); | |||
There was a problem hiding this comment.
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; | |||
There was a problem hiding this comment.
Shouldn't we just throw error here?
There was a problem hiding this comment.
that was debug. I need to remove it.
No description provided.