Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
import org.apache.gobblin.metrics.event.lineage.LineageInfo;
import org.apache.gobblin.source.extractor.extract.EventBasedSource;
import org.apache.gobblin.source.extractor.extract.kafka.workunit.packer.KafkaWorkUnitPacker;
import org.apache.gobblin.source.extractor.extract.kafka.validator.TopicValidators;
import org.apache.gobblin.source.extractor.limiter.LimiterConfigurationKeys;
import org.apache.gobblin.source.workunit.Extract;
import org.apache.gobblin.source.workunit.MultiWorkUnit;
Expand Down Expand Up @@ -218,7 +219,7 @@ public List<WorkUnit> getWorkunits(SourceState state) {

this.kafkaConsumerClient.set(kafkaConsumerClientFactory.create(config));

List<KafkaTopic> topics = getFilteredTopics(state);
List<KafkaTopic> topics = getValidTopics(getFilteredTopics(state), state);
this.topicsToProcess = topics.stream().map(KafkaTopic::getName).collect(toSet());

for (String topic : this.topicsToProcess) {
Expand Down Expand Up @@ -802,6 +803,7 @@ private WorkUnit getWorkUnitForTopicPartition(KafkaPartition partition, Offsets
protected List<KafkaTopic> getFilteredTopics(SourceState state) {
List<Pattern> blacklist = DatasetFilterUtils.getPatternList(state, TOPIC_BLACKLIST);
List<Pattern> whitelist = DatasetFilterUtils.getPatternList(state, TOPIC_WHITELIST);
// TODO: replace this with TopicNameValidator in the config once TopicValidators is rolled out.
if (!state.getPropAsBoolean(KafkaSource.ALLOW_PERIOD_IN_TOPIC_NAME, true)) {
blacklist.add(Pattern.compile(".*\\..*"));
}
Expand All @@ -815,6 +817,13 @@ public void shutdown(SourceState state) {
state.setProp(ConfigurationKeys.FAIL_TO_GET_OFFSET_COUNT, this.failToGetOffsetCount);
}

/**
* Return topics that pass all the topic validators.
*/
protected List<KafkaTopic> getValidTopics(List<KafkaTopic> topics, SourceState state) {
return new TopicValidators(state).validate(topics);
}

/**
* This class contains startOffset, earliestOffset and latestOffset for a Kafka partition.
*/
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.apache.gobblin.source.extractor.extract.kafka.validator;

import org.apache.gobblin.configuration.SourceState;
import org.apache.gobblin.source.extractor.extract.kafka.KafkaTopic;

/**
* A topic validator that validates the topic name
*/
public class TopicNameValidator extends TopicValidatorBase {
private static final String DOT = ".";

public TopicNameValidator(SourceState sourceState) {
super(sourceState);
}

/**
* Check if a topic name is valid, current rules are:
* 1. must not contain "."
* @param topic the topic to be validated
* @return true if the topic name is valid (aka. doesn't contain ".")
*/
@Override
public boolean validate(KafkaTopic topic) {
return !topic.getName().contains(DOT);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.apache.gobblin.source.extractor.extract.kafka.validator;

import org.apache.gobblin.configuration.SourceState;
import org.apache.gobblin.source.extractor.extract.kafka.KafkaTopic;

/**
* The base class of a topic validator
*/
public abstract class TopicValidatorBase {
protected SourceState sourceState;

public TopicValidatorBase(SourceState sourceState) {
this.sourceState = sourceState;
}

public abstract boolean validate(KafkaTopic topic);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.apache.gobblin.source.extractor.extract.kafka.validator;

import java.util.ArrayList;
import java.util.List;
import java.util.stream.Collectors;
import lombok.extern.slf4j.Slf4j;
import org.apache.commons.lang.StringUtils;
import org.apache.gobblin.configuration.SourceState;
import org.apache.gobblin.source.extractor.extract.kafka.KafkaTopic;
import org.apache.gobblin.util.reflection.GobblinConstructorUtils;

/**
* The TopicValidators contains a list of {@link TopicValidatorBase} that validate topics.
* To enable it, add below settings in the config:
* gobblin.kafka.topicValidators=validator1_class_name,validator2_class_name...
*/
@Slf4j
public class TopicValidators {
public static final String VALIDATOR_CLASSES_KEY = "gobblin.kafka.topicValidators";

public static final String VALIDATOR_CLASS_DELIMITER = ",";

private final List<TopicValidatorBase> validators = new ArrayList<>();

public TopicValidators(SourceState state) {
for (String validatorClassName : state.getPropAsList(VALIDATOR_CLASSES_KEY, StringUtils.EMPTY)) {
try {
this.validators.add(GobblinConstructorUtils.invokeConstructor(TopicValidatorBase.class, validatorClassName,
state));
} catch (Exception e) {
log.error("Failed to create topic validator: {}, due to {}", validatorClassName, e);
}
}
}

/**
* Validate topics with all the internal validators.
* Note: the validations for every topic run in parallel.
* @param topics the topics to be validated
* @return the topics that pass all the validators
*/
public List<KafkaTopic> validate(List<KafkaTopic> topics) {
// Validate the topics in parallel
return topics.parallelStream()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not that I think this class will be used within Incremental Compute, but just FYI, we have seen in the past where parallelStream has caused some strange bugs related to not picking up dependencies in Spark. In general, I'd suggest always thinking about whether the parallelism is important given that the api has caused issue in the past.

#3706

@wsarecv wsarecv Oct 11, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the reference of that PR.

Yes, the parallelism is on purpose. Some validator could be slow, validating the topics sequentially would significantly slow down the startup.

For example, there will be another PR to add an OrcSchemaConversionValidator that queries the remote schema registry for every topic.

If the parallelStream is not recommended, we would have to do it in a separate thread pool while trying to avoid the context class loader issue. Any comment on this approach?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we intend to keep this code limited to Kafka source? i.e. do we see a world where we'd want to use this topic validation in incremental?..

If we do see spark uses cases, could we look into the following comment from that PR that describes an alternative approach that would set the appropriate class loader? This sort of topic validation could make sense since Iceberg does not support some of our weird edge cases (e.g. non optional union schema)

#3706 (comment)

JFYI, in compaction we do have a similar validation step at the dataset finder level (and the PR I linked previously is a bug fix for that feature). If the intention is to use this topic validation across the board you can consider whether it makes sense to converge these code paths.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ideally, the data from a bad topic should not come to the incremental compaction stage, because:

  1. With static validators, KafkaSource can skip the bad topics during the work unit creation.
  2. With dynamic validators, bad topics are detected on the fly and then FastIngest can be restarted to skip those bad topics.

However, the incremental compaction may potentially use a different set of validators, so it still make sense to provide the validation support beyond FastIngest. So let's go for it.

To set the context class loader correctly, in the new iteration the ExecutorsUtils is enhanced to help create thread pools where the running tasks can have the same access control and class loader settings as the thread that submits the tasks. Please help take a look.

.filter(this::validate)
.collect(Collectors.toList());
}

/**
* Validates a single topic with all the internal validators
*/
private boolean validate(KafkaTopic topic) {
log.debug("Validating topic {} in thread: {}", topic, Thread.currentThread().getName());
for (TopicValidatorBase validator : this.validators) {
if (!validator.validate(topic)) {
log.info("Skip KafkaTopic: {}, by validator: {}", topic, validator.getClass().getName());
return false;
}
}
return true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@
import java.util.regex.Pattern;
import java.util.stream.Collectors;

import org.apache.commons.collections.CollectionUtils;
import org.apache.gobblin.source.extractor.extract.kafka.validator.TopicNameValidator;
import org.apache.gobblin.source.extractor.extract.kafka.validator.TopicValidators;
import org.testng.Assert;
import org.testng.annotations.Test;

Expand Down Expand Up @@ -56,6 +59,36 @@ public void testGetFilteredTopics() {
Assert.assertEquals(new TestKafkaSource(testKafkaClient).getFilteredTopics(state), toKafkaTopicList(allTopics.subList(0, 3)));
}

@Test
public void testTopicValidators() {
TestKafkaClient testKafkaClient = new TestKafkaClient();
List<String> allTopics = Arrays.asList(
"Topic1", "topic-v2", "topic3", // allowed
"topic-with.period-in_middle", ".topic-with-period-at-start", "topicWithPeriodAtEnd.", //period topics
"not-allowed-topic");
testKafkaClient.testTopics = allTopics;
KafkaSource kafkaSource = new TestKafkaSource(testKafkaClient);

SourceState state = new SourceState();
state.setProp(KafkaSource.TOPIC_WHITELIST, ".*[Tt]opic.*");
state.setProp(KafkaSource.TOPIC_BLACKLIST, "not-allowed.*");
List<KafkaTopic> topicsToValidate = kafkaSource.getFilteredTopics(state);

// Test without TopicValidators in the state
Assert.assertTrue(CollectionUtils.isEqualCollection(kafkaSource.getValidTopics(topicsToValidate, state),
toKafkaTopicList(allTopics.subList(0, 6))));

// Test empty TopicValidators in the state
state.setProp(TopicValidators.VALIDATOR_CLASSES_KEY, "");
Assert.assertTrue(CollectionUtils.isEqualCollection(kafkaSource.getValidTopics(topicsToValidate, state),
toKafkaTopicList(allTopics.subList(0, 6))));

// Test TopicValidators with TopicNameValidator in the state
state.setProp(TopicValidators.VALIDATOR_CLASSES_KEY, TopicNameValidator.class.getName());
Assert.assertTrue(CollectionUtils.isEqualCollection(kafkaSource.getValidTopics(topicsToValidate, state),
toKafkaTopicList(allTopics.subList(0, 3))));
}

public List<KafkaTopic> toKafkaTopicList(List<String> topicNames) {
return topicNames.stream().map(topicName -> new KafkaTopic(topicName, Collections.emptyList())).collect(Collectors.toList());
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.apache.gobblin.source.extractor.extract.kafka.validator;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Set;
import java.util.stream.Collectors;
import org.apache.gobblin.configuration.SourceState;
import org.apache.gobblin.source.extractor.extract.kafka.KafkaTopic;
import org.testng.Assert;
import org.testng.annotations.Test;


public class TopicValidatorsTest {
@Test
public void testTopicValidators() {
List<String> allTopics = Arrays.asList(
"topic1", "topic2", // allowed
"topic-with.period-in_middle", ".topic-with-period-at-start", "topicWithPeriodAtEnd.", // bad topics
"topic3", "topic4"); // in deny list
List<KafkaTopic> topics = allTopics.stream()
.map(topicName -> new KafkaTopic(topicName, Collections.emptyList())).collect(Collectors.toList());

SourceState state = new SourceState();

// Without any topic validators
List<KafkaTopic> validTopics = new TopicValidators(state).validate(topics);
Assert.assertEquals(validTopics.size(), 7);

// Use 2 topic validators: TopicNameValidator and DenyListValidator
String validatorsToUse = String.join(TopicValidators.VALIDATOR_CLASS_DELIMITER,
ImmutableList.of(TopicNameValidator.class.getName(), DenyListValidator.class.getName()));
state.setProp(TopicValidators.VALIDATOR_CLASSES_KEY, validatorsToUse);
validTopics = new TopicValidators(state).validate(topics);

Assert.assertEquals(validTopics.size(), 2);
Assert.assertTrue(validTopics.stream().anyMatch(topic -> topic.getName().equals("topic1")));
Assert.assertTrue(validTopics.stream().anyMatch(topic -> topic.getName().equals("topic2")));
}

// A TopicValidator class to mimic a deny list
public static class DenyListValidator extends TopicValidatorBase {
Set<String> denyList = ImmutableSet.of("topic3", "topic4");

public DenyListValidator(SourceState sourceState) {
super(sourceState);
}

@Override
public boolean validate(KafkaTopic topic) {
return !this.denyList.contains(topic.getName());
}
}
}