Pull config changes, ephemeral pull, unit tests #645
Conversation
scottf
commented
May 21, 2022
•
scottf
commented
- architecture issue 112 Pull config changes in ConsumerConfig and JSApiConsumerGetNextRequest
- remove enforcement of durable on pull subscriptions allowing ephemeral pull consumers
- unit tests for above and additional unit tests to shore up coverage
…iConsumerGetNextRequest
…iConsumerGetNextRequest
| @@ -45,26 +45,6 @@ public String toJson() { | |||
| return endJson(sb).toString(); | |||
| } | |||
|
|
|||
| @Override | |||
| public boolean equals(Object o) { | |||
| if (this == o) return true; | |||
There was a problem hiding this comment.
Not needed, never used the object is not compared with anything. Not sure why this was added in the first place.
| protected final Integer maxAckPending; | ||
| protected final Integer maxPullWaiting; | ||
| protected final Integer maxBatch; | ||
| protected final Integer maxBytes; |
There was a problem hiding this comment.
After reviewing the server code I realized that the fields were go int, which are 32 bit signed values aka java int. I have only changed the internal variable type, getters and builders were left the same.
|
|
||
| if (l > Integer.MAX_VALUE) { | ||
| return Integer.MAX_VALUE; | ||
| } |
There was a problem hiding this comment.
A value for these integer fields anywhere near Integer MAX_VALUE would not make sense, so instead of an error if the proposed long value was beyond that, I just assumed the user was trying to set some max value and handled it instead of erroring.
| @@ -1055,27 +1116,25 @@ private static Duration normalizeDuration(long millis) | |||
| * INTERNAL CLASS ONLY, SUBJECT TO CHANGE | |||
| * Helper class to manage min / default / unset / server values. | |||
| */ | |||
| public enum LongChangeHelper { | |||
| MAX_DELIVER(1, -1); // 0 is treated the same as -1 on the server, which is why the server doesn't omit this | |||
| public enum IntegerChangeHelper { | |||
There was a problem hiding this comment.
reflecting that it's addressing integers, not longs
| if (userCC.maxBytesWasSet()) { | ||
| throw JsSubPushCantHaveMaxBytes.instance(); | ||
| } | ||
|
|
There was a problem hiding this comment.
new field added so new validation added.
README.md
Outdated
| @@ -588,6 +588,7 @@ Subscription creation has many checks to make sure that a valid, operable subscr | |||
| | JsSubConsumerNotFoundRequiredInBind | SUB | 90017 | Consumer not found, required in bind mode. | | |||
| | JsSubOrderedNotAllowOnQueues | SUB | 90018 | Ordered consumer not allowed on queues. | | |||
| | JsSubPushCantHaveMaxBatch | SUB | 90019 | Push subscriptions cannot supply max batch. | | |||
| | JsSubPushCantHaveMaxBatch | SUB | 90020 | Push subscriptions cannot supply max bytes. | | |||
There was a problem hiding this comment.
new field added so new validation added.
| JsonUtils.addField(sb, "batch", batch); | ||
| JsonUtils.addFldWhenTrue(sb, "no_wait", noWait); | ||
| JsonUtils.addFieldAsNanos(sb, "expires", expiresIn); | ||
| return JsonUtils.endJson(sb).toString().getBytes(StandardCharsets.US_ASCII); |
There was a problem hiding this comment.
_pull and getPullJson are not needed because the json is done in the PullRequestOptions object and the _pull code is moved to the new api that directly takes the PullRequestOptions since the other pulls can delegate to it now
| @Override | ||
| public void pull(PullRequestOptions pullRequestOptions) { | ||
| throw new IllegalStateException(SUBSCRIPTION_TYPE_DOES_NOT_SUPPORT_PULL); | ||
| } |
There was a problem hiding this comment.
Only pull subs allow this call and this is the base class, not the pull version so is the default behavior. For simplification, the interface design should be more like .NET where the pull interface is not part of the JS Sub interface
| @@ -23,22 +23,13 @@ class PullStatusMessageManager extends MessageManager { | |||
|
|
|||
| private static final List<Integer> PULL_KNOWN_STATUS_CODES = Arrays.asList(404, 408, 409); | |||
|
|
|||
| private int lastStatusCode = -1; | |||
There was a problem hiding this comment.
this field an associated getter were not used as noted by the IDE. So removed them.
| @@ -101,7 +101,7 @@ public void testChangeFieldsIdentified() { | |||
| assertNotChange(builder(orig).startSequence(null).build(), orig); | |||
| assertChange(builder(orig).startSequence(1).build(), orig, "startSequence"); | |||
|
|
|||
| assertNotChange(builder(orig).maxDeliver(MAX_DELIVER.Unset).build(), orig); | |||
| assertNotChange(builder(orig).maxDeliver(INTEGER_UNSET).build(), orig); | |||
There was a problem hiding this comment.
the Unset field has been removed from the object in place of using a constant.
…any issue reading an int into a long
| @@ -281,8 +290,8 @@ public Duration getAckWait() { | |||
| * Gets the max delivery amount of this consumer configuration. | |||
| * @return the max delivery amount. | |||
| */ | |||
| public long getMaxDeliver() { | |||
| return LongChangeHelper.MAX_DELIVER.getOrUnset(maxDeliver); | |||
| public int getMaxDeliver() { | |||
There was a problem hiding this comment.
changed getters to return int since the underlying value is an int. This is not a breaking change since it is a primitive and used to be primitive long, so there would be no break if the user had read this value into an long variable.
applies to getMaxDeliver, getMaxAckPending, getMaxPullWaiting, getMaxBatch, getMaxBytes
scottf
changed the title
…l pull consumers and additional unit tests to shore up coverage
