-
Notifications
You must be signed in to change notification settings - Fork 292
extend the validations: retention period mandatory for materialisatio… #1543
extend the validations: retention period mandatory for materialisatio… #1543
Conversation
...-common/src/test/java/org/zalando/nakadi/annotations/validation/DataLakeAnnotationsTest.java
Outdated
Show resolved
Hide resolved
👍 |
...mon/src/main/java/org/zalando/nakadi/annotations/validation/DataLakeAnnotationValidator.java
Outdated
Show resolved
Hide resolved
👍 |
1 similar comment
👍 |
Note: please don't roll out, it is breaking backward compatibility right now. There should be an exception for the affected events. |
…requested teams in order to not break backward compatibility
…erialisation opt in
if (annotations.containsKey(materialiseEventsAnnotationText)) { | ||
if (annotations.get(materialiseEventsAnnotationText).equals("on") && | ||
!annotations.containsKey(retentionPeriodAnnotation)) { | ||
annotations.put(retentionPeriodAnnotation, "unlimited"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not do that — the API should accept and store the annotations exactly as provided by the clients.
Can you think of a different way to achieve the same effect?
@@ -521,8 +534,43 @@ private void updateRetentionTime(final EventType original, final EventType event | |||
private void updateAnnotationsAndLabels(final EventType original, final EventType eventType) | |||
throws InvalidEventTypeException { | |||
|
|||
/* this is a temporary solution to ensure the backward compatibility and it is here because | |||
the DataLakeAnnotationValidator.java does not have access to the event type */ | |||
final List<String> breakingCompatibilitiesNames = Arrays.asList( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adrianafirutanasie please remove the company-specific names from the public GitHub repository.
final Map<String, String> originalAnnotations = original.getAnnotations(); | ||
if (breakingCompatibilitiesNames.contains(original.getName())){ | ||
if (!originalAnnotations.containsKey("datalake.zalando.org/retention-period")) { | ||
originalAnnotations.put("datalake.zalando.org/retention-period", "unlimited"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, we should not be doing this: the variable is called original annotations for a reason.
DataLakeAnnotationValidator.MATERIALISE_EVENTS_ANNOTATION, "on" | ||
); | ||
final Set<ConstraintViolation<TestClass>> result = validator.validate(new TestClass(annotations)); | ||
assertTrue(DataLakeAnnotationValidator.RETENTION_PERIOD_ANNOTATION + " is specified.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retention Period Annotation is required
, not specified
@@ -191,6 +192,17 @@ public void create(final EventTypeBase eventType, final boolean checkAuth) | |||
if (eventType.getAnnotations() == null) { | |||
eventType.setAnnotations(new HashMap<>()); | |||
} | |||
else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's delete this part, it's not needed anymore
@@ -19,17 +19,29 @@ public boolean isValid(final Map<String, String> annotations, final ConstraintVa | |||
if (annotations == null || annotations.size() == 0) { | |||
return true; | |||
} | |||
final String materialiseEventsAnnotation = annotations.get(MATERIALISE_EVENTS_ANNOTATION); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use materialize
, because we use the US English.
@eliseealex there is work in this direction in different PR #1587 |
Comments are addressed in the: #1543, Danylo had no access to this PR, could you close it and use the new one instead. |
extend the validations: retention period mandatory for materialisation opt-in
One-line summary
Description
We have extended the validations for the annotations field, to make the retention period field mandatory when the materialise_events field has value "on"
Review