Fragment validator interface & implementation #735
Conversation
| @@ -170,7 +173,7 @@ public final class Engine implements Collector, AutoCloseable | |||
| DispatchAgent agent = | |||
| new DispatchAgent(config, tasks, labels, errorHandler, tuning::affinity, | |||
| bindings, exporters, guards, vaults, catalogs, metricGroups, converterFactory, | |||
| this, coreIndex, readonly); | |||
| validatorFactory, this, coreIndex, readonly); | |||
There was a problem hiding this comment.
If possible, converters and validators should follow the same pattern as the others, passing a collection instead of a factory.
| @@ -139,6 +141,9 @@ Converter createReader( | |||
| Converter createWriter( | |||
| ConverterConfig converter); | |||
|
|
|||
| Validator supplyValidator( | |||
| ValidatorConfig converter); | |||
|
|
|||
There was a problem hiding this comment.
Please move this up next to other supplyXXX methods for guard, vault and catalog.
Also, shouldn't this be returning a ValidatorHandler (per core) instead of a Validator (global) given that it is on EngineContext which is per core?
|
|
||
| Validator create( | ||
| ValidatorConfig config, | ||
| LongFunction<CatalogHandler> supplyCatalog); |
There was a problem hiding this comment.
This seems inconsistent, as Validator is global, whereas CatalogHandler is per core.
| public String type() | ||
| { | ||
| return "string"; | ||
| } |
There was a problem hiding this comment.
Please move this to first method position for clarity.
| String encoding = object.containsKey(ENCODING_NAME) | ||
| ? object.getString(ENCODING_NAME) | ||
| : null; | ||
| result = new StringValidatorConfig(encoding); |
There was a problem hiding this comment.
Use builder for both cases.
Also use enum returned from value.getValueType() in a switch instead of if-else with instanceof.
| else | ||
| { | ||
| assert false; | ||
| } |
There was a problem hiding this comment.
No need for this assert, covered by schema.
| public ValidatorContext supply( | ||
| EngineContext context) | ||
| { | ||
| return new StringValidatorContext(); |
There was a problem hiding this comment.
Usually we pass the engine context in the constructor.
|
|
||
| public StringValidator( | ||
| StringValidatorConfig config, | ||
| LongFunction<CatalogHandler> supplyCatalog) |
There was a problem hiding this comment.
No need to pass supplyCatalog here if we can call it from EngineContext passed to supply method instead?
| private int incompleteCharCount; | ||
| private int incompleteChar; | ||
|
|
||
| public StringValidatorHandler(StringValidatorConfig config) |
There was a problem hiding this comment.
Formatting and naturally need to pull out the encoding from the config.
There was a problem hiding this comment.
| public StringValidatorHandler(StringValidatorConfig config) | |
| public StringValidatorHandler( | |
| StringValidatorConfig config) |
...or/types-core/src/main/java/io/aklivity/zilla/runtime/types/core/StringValidatorHandler.java
Outdated
Show resolved
Hide resolved
| private int incompleteCharCount; | ||
| private int incompleteChar; | ||
|
|
||
| public StringValidatorHandler(StringValidatorConfig config) |
There was a problem hiding this comment.
| public StringValidatorHandler(StringValidatorConfig config) | |
| public StringValidatorHandler( | |
| StringValidatorConfig config) |
| final int charByteCount = (charByte0 & 0b1000_0000) != 0 | ||
| ? Integer.numberOfLeadingZeros((~charByte0 & 0xff) << 24) | ||
| : 1; |
There was a problem hiding this comment.
charByteCount should move to the else clause as it is not needed for the if clause.
| @@ -77,31 +73,19 @@ public boolean validate( | |||
| } | |||
| } | |||
| index += charByteCount; | |||
There was a problem hiding this comment.
Feels like we are double counting the char bytes when fragmented, since index++ will happen for each pending byte and index += charByteCount will happen for complete chars.
Perhaps we can simplify this algorithm to work from the perspective of pendingBytes always, and handle complete chars as a special case, where we need to reinitialize the number of pendingBytes from the first character byte?
| if ((flags & FLAGS_FIN) != 0x00) | ||
| { | ||
| valid = initialize && incompleteCharCount == 0 && index == limit; | ||
| initialize = false; | ||
| valid = pendingCharBytes == 0 && index == limit; | ||
| } | ||
| else | ||
| { | ||
| valid = initialize && index == limit; | ||
| valid = index + pendingCharBytes == limit; | ||
| } |
There was a problem hiding this comment.
Suggest simplifying to a ternary operator.
value = (flags & FLAGS_FIN) == 0x00
? index + pendingCharBytes == limit
: pendingCharBytes == 0 && index == limit;
Btw, why is index + pendingCharBytes == limit the correct expression for fragmented messages? The index and length represent a single fragment, so why does that relate to pendingBytes?
| StringValidatorConfig result = null; | ||
| if (value instanceof JsonString) | ||
| switch (type) |
There was a problem hiding this comment.
| switch (type) | |
| switch (value.valueType()) |
| @@ -53,29 +58,23 @@ public JsonValue adaptToJson( | |||
| public StringValidatorConfig adaptFromJson( | |||
| JsonValue value) | |||
| { | |||
| JsonValue.ValueType type = value.getValueType(); | |||
| StringValidatorConfig result = null; | |||
There was a problem hiding this comment.
Use config instead of result?
| @@ -19,14 +19,9 @@ | |||
|
|
|||
| public interface ValidatorContext | |||
| { | |||
| default ValidatorHandler attach( | |||
| default ValidatorHandler supplyHandler( | |||
There was a problem hiding this comment.
I think we need to remove the default and { return null; } as this must always be implemented.
| } | ||
| }, | ||
|
|
||
| INVALID |
There was a problem hiding this comment.
If schema requires utf_8 as only possibility, this can be removed, agree?
| * WARRANTIES OF ANY KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations under the License. | ||
| */ | ||
| package io.aklivity.zilla.runtime.types.core; |
There was a problem hiding this comment.
All non-public code should be in internal subpackage like other modules.
No description provided.