-
Notifications
You must be signed in to change notification settings - Fork 2.5k
[HUDI-4445] S3 Incremental source improvements #6176
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
e434c6b
0ff79e1
e783a88
3934625
e9fd966
d1d558f
96af133
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| package org.apache.hudi.common.testutils; | ||
|
|
||
| import org.apache.avro.Schema; | ||
| import org.apache.avro.SchemaBuilder; | ||
| import org.apache.avro.generic.GenericData; | ||
| import org.apache.avro.generic.GenericRecord; | ||
|
|
||
| // Utility for the schema of S3 events listed here (https://docs.aws.amazon.com/AmazonS3/latest/userguide/notification-content-structure.html) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should be multi-line comment
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be multi line comment. |
||
| public class S3EventsSchemaUtils { | ||
| public static final String DEFAULT_STRING_VALUE = "default_string"; | ||
|
|
||
| public static String generateSchemaString() { | ||
| return generateS3EventSchema().toString(); | ||
| } | ||
|
|
||
| public static Schema generateObjInfoSchema() { | ||
| Schema objInfo = SchemaBuilder.record("objInfo") | ||
| .fields() | ||
| .requiredString("key") | ||
| .requiredLong("size") | ||
| .endRecord(); | ||
| return objInfo; | ||
| } | ||
|
|
||
| public static GenericRecord generateObjInfoRecord(String key, Long size) { | ||
| GenericRecord rec = new GenericData.Record(generateObjInfoSchema()); | ||
| rec.put("key", key); | ||
| rec.put("size", size); | ||
| return rec; | ||
| } | ||
|
|
||
| public static Schema generateS3MetadataSchema() { | ||
| Schema s3Metadata = SchemaBuilder.record("s3Metadata") | ||
| .fields() | ||
| .requiredString("configurationId") | ||
| .name("object") | ||
| .type(generateObjInfoSchema()) | ||
| .noDefault() | ||
| .endRecord(); | ||
| return s3Metadata; | ||
| } | ||
|
|
||
| public static GenericRecord generateS3MetadataRecord(GenericRecord objRecord) { | ||
| GenericRecord rec = new GenericData.Record(generateS3MetadataSchema()); | ||
| rec.put("configurationId", DEFAULT_STRING_VALUE); | ||
| rec.put("object", objRecord); | ||
| return rec; | ||
| } | ||
|
|
||
| public static Schema generateS3EventSchema() { | ||
| Schema s3Event = SchemaBuilder.record("s3Event") | ||
| .fields() | ||
| .requiredString("eventSource") | ||
| .requiredString("eventName") | ||
| .name("s3") | ||
|
Comment on lines
+53
to
+93
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's extract all these strings to constants.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Preferably extract all these as static string constants. |
||
| .type(generateS3MetadataSchema()) | ||
| .noDefault() | ||
| .endRecord(); | ||
| return s3Event; | ||
| } | ||
|
|
||
| public static GenericRecord generateS3EventRecord(String rowKey, GenericRecord objRecord) { | ||
| GenericRecord rec = new GenericData.Record(generateS3EventSchema()); | ||
| rec.put("_row_key", rowKey); | ||
| rec.put("eventSource", DEFAULT_STRING_VALUE); | ||
| rec.put("eventName", DEFAULT_STRING_VALUE); | ||
| rec.put("s3", generateS3MetadataRecord(objRecord)); | ||
| return rec; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ | |
|
|
||
| import org.apache.hudi.DataSourceReadOptions; | ||
| import org.apache.hudi.DataSourceUtils; | ||
| import org.apache.hudi.common.config.SerializableConfiguration; | ||
| import org.apache.hudi.common.config.TypedProperties; | ||
| import org.apache.hudi.common.fs.FSUtils; | ||
| import org.apache.hudi.common.model.HoodieRecord; | ||
|
|
@@ -32,6 +33,7 @@ | |
|
|
||
| import com.esotericsoftware.minlog.Log; | ||
| import com.fasterxml.jackson.databind.ObjectMapper; | ||
| import org.apache.hadoop.conf.Configuration; | ||
| import org.apache.hadoop.fs.FileSystem; | ||
| import org.apache.hadoop.fs.Path; | ||
| import org.apache.log4j.LogManager; | ||
|
|
@@ -43,10 +45,13 @@ | |
| import org.apache.spark.sql.SparkSession; | ||
|
|
||
| import java.io.IOException; | ||
| import java.net.URLDecoder; | ||
| import java.nio.charset.StandardCharsets; | ||
| import java.util.ArrayList; | ||
| import java.util.Collections; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| import static org.apache.hudi.utilities.sources.HoodieIncrSource.Config.DEFAULT_NUM_INSTANTS_PER_FETCH; | ||
| import static org.apache.hudi.utilities.sources.HoodieIncrSource.Config.DEFAULT_READ_LATEST_INSTANT_ON_MISSING_CKPT; | ||
|
|
@@ -172,37 +177,51 @@ public Pair<Option<Dataset<Row>>, String> fetchNextBatch(Option<String> lastCkpt | |
| String s3FS = props.getString(Config.S3_FS_PREFIX, "s3").toLowerCase(); | ||
| String s3Prefix = s3FS + "://"; | ||
|
|
||
| // Extract distinct file keys from s3 meta hoodie table | ||
| final List<Row> cloudMetaDf = source | ||
| // Create S3 paths | ||
| final boolean checkExists = props.getBoolean(Config.ENABLE_EXISTS_CHECK, Config.DEFAULT_ENABLE_EXISTS_CHECK); | ||
| SerializableConfiguration serializableConfiguration = new SerializableConfiguration(sparkContext.hadoopConfiguration()); | ||
| List<String> cloudFiles = source | ||
| .filter(filter) | ||
| .select("s3.bucket.name", "s3.object.key") | ||
| .distinct() | ||
| .collectAsList(); | ||
| // Create S3 paths | ||
| final boolean checkExists = props.getBoolean(Config.ENABLE_EXISTS_CHECK, Config.DEFAULT_ENABLE_EXISTS_CHECK); | ||
| List<String> cloudFiles = new ArrayList<>(); | ||
| for (Row row : cloudMetaDf) { | ||
| // construct file path, row index 0 refers to bucket and 1 refers to key | ||
| String bucket = row.getString(0); | ||
| String filePath = s3Prefix + bucket + "/" + row.getString(1); | ||
| if (checkExists) { | ||
| FileSystem fs = FSUtils.getFs(s3Prefix + bucket, sparkSession.sparkContext().hadoopConfiguration()); | ||
| try { | ||
| if (fs.exists(new Path(filePath))) { | ||
| cloudFiles.add(filePath); | ||
| } | ||
| } catch (IOException e) { | ||
| LOG.error(String.format("Error while checking path exists for %s ", filePath), e); | ||
| } | ||
| } else { | ||
| cloudFiles.add(filePath); | ||
| } | ||
| } | ||
| .rdd() | ||
| // JavaRDD simplifies coding with collect and suitable mapPartitions signature. check if this can be avoided. | ||
| .toJavaRDD() | ||
| .mapPartitions(fileListIterator -> { | ||
| List<String> cloudFilesPerPartition = new ArrayList<>(); | ||
| fileListIterator.forEachRemaining(row -> { | ||
| // TODO: configuration is updated in the getFs call. check if new copy is needed w.r.t to getFs. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this still required? |
||
| final Configuration configuration = serializableConfiguration.newCopy(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why creating a copy again? I don't see any config modification happening within the executor. Why not pass |
||
| String bucket = row.getString(0); | ||
| String filePath = s3Prefix + bucket + "/" + row.getString(1); | ||
| try { | ||
| String decodeUrl = URLDecoder.decode(filePath, StandardCharsets.UTF_8.name()); | ||
| if (checkExists) { | ||
| FileSystem fs = FSUtils.getFs(s3Prefix + bucket, configuration); | ||
| try { | ||
| if (fs.exists(new Path(decodeUrl))) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. creating hadoop Path gives much more memory overhead than normal instantiation. If just for checking, let's find a better way. |
||
| cloudFilesPerPartition.add(decodeUrl); | ||
| } | ||
| } catch (IOException e) { | ||
| LOG.error(String.format("Error while checking path exists for %s ", decodeUrl), e); | ||
| } | ||
| } else { | ||
| cloudFilesPerPartition.add(decodeUrl); | ||
| } | ||
| } catch (Exception exception) { | ||
| LOG.warn("Failed to add cloud file ", exception); | ||
| } | ||
| }); | ||
| return cloudFilesPerPartition.iterator(); | ||
| }).collect(); | ||
|
|
||
| Option<Dataset<Row>> dataset = Option.empty(); | ||
| if (!cloudFiles.isEmpty()) { | ||
| DataFrameReader dataFrameReader = getDataFrameReader(fileFormat); | ||
| dataset = Option.of(dataFrameReader.load(cloudFiles.toArray(new String[0]))); | ||
| } | ||
| LOG.debug("Extracted distinct files " + cloudFiles.size() | ||
| + " and some samples " + cloudFiles.stream().limit(10).collect(Collectors.toList())); | ||
| return Pair.of(dataset, queryTypeAndInstantEndpts.getRight().getRight()); | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
-229
to
+237
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should have the EOL |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,134 @@ | ||
| package org.apache.hudi.utilities.sources; | ||
|
|
||
| import org.apache.avro.Schema; | ||
| import org.apache.hudi.client.SparkRDDWriteClient; | ||
| import org.apache.hudi.client.WriteStatus; | ||
| import org.apache.hudi.common.config.HoodieMetadataConfig; | ||
| import org.apache.hudi.common.config.TypedProperties; | ||
| import org.apache.hudi.common.model.HoodieRecord; | ||
| import org.apache.hudi.common.table.HoodieTableMetaClient; | ||
| import org.apache.hudi.common.table.timeline.versioning.TimelineLayoutVersion; | ||
| import org.apache.hudi.common.testutils.HoodieTestDataGenerator; | ||
| import org.apache.hudi.common.testutils.S3EventsSchemaUtils; | ||
| import org.apache.hudi.common.util.Option; | ||
| import org.apache.hudi.common.util.collection.Pair; | ||
| import org.apache.hudi.config.HoodieArchivalConfig; | ||
| import org.apache.hudi.config.HoodieCleanConfig; | ||
| import org.apache.hudi.config.HoodieWriteConfig; | ||
| import org.apache.hudi.testutils.SparkClientFunctionalTestHarness; | ||
| import org.apache.hudi.utilities.schema.SchemaProvider; | ||
| import org.apache.hudi.utilities.sources.helpers.IncrSourceHelper; | ||
| import org.apache.spark.api.java.JavaRDD; | ||
| import org.apache.spark.sql.Dataset; | ||
| import org.apache.spark.sql.Row; | ||
| import org.junit.jupiter.api.Assertions; | ||
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.List; | ||
| import java.util.Properties; | ||
|
|
||
| import static org.apache.hudi.common.testutils.HoodieTestDataGenerator.S3_EVENTS_SCHEMA; | ||
| import static org.apache.hudi.testutils.Assertions.assertNoWriteErrors; | ||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertFalse; | ||
|
|
||
| public class TestS3EventsHoodieIncrSource extends SparkClientFunctionalTestHarness { | ||
| private HoodieTestDataGenerator dataGen; | ||
| private HoodieTableMetaClient metaClient; | ||
|
|
||
| @BeforeEach | ||
| public void setUp() throws IOException { | ||
| dataGen = new HoodieTestDataGenerator(); | ||
| metaClient = getHoodieMetaClient(hadoopConf(), basePath()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testHoodieIncrSource() throws IOException { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe rename to testS3EventsHoodieIncrSource? |
||
| HoodieWriteConfig writeConfig = getConfigBuilder(basePath(), metaClient) | ||
| .withArchivalConfig(HoodieArchivalConfig.newBuilder().archiveCommitsWith(2, 3).build()) | ||
| .withCleanConfig(HoodieCleanConfig.newBuilder().retainCommits(1).build()) | ||
| .withMetadataConfig(HoodieMetadataConfig.newBuilder() | ||
| .withMaxNumDeltaCommitsBeforeCompaction(1).build()) | ||
| .build(); | ||
|
|
||
| SparkRDDWriteClient writeClient = getHoodieWriteClient(writeConfig); | ||
| Pair<String, List<HoodieRecord>> inserts = writeRecords(writeClient, null, "100"); | ||
| Pair<String, List<HoodieRecord>> inserts2 = writeRecords(writeClient, null, "200"); | ||
| Pair<String, List<HoodieRecord>> inserts3 = writeRecords(writeClient, null, "300"); | ||
| Pair<String, List<HoodieRecord>> inserts4 = writeRecords(writeClient, null, "400"); | ||
| Pair<String, List<HoodieRecord>> inserts5 = writeRecords(writeClient, null, "500"); | ||
|
|
||
| // read everything upto latest | ||
| readAndAssert(IncrSourceHelper.MissingCheckpointStrategy.READ_UPTO_LATEST_COMMIT, Option.empty(), 500, inserts5.getKey()); | ||
|
|
||
| // even if the begin timestamp is archived (100), full table scan should kick in, but should filter for records having commit time > 100 | ||
| readAndAssert(IncrSourceHelper.MissingCheckpointStrategy.READ_UPTO_LATEST_COMMIT, Option.of("100"), 400, inserts5.getKey()); | ||
|
|
||
| // even if the read upto latest is set, if begin timestamp is in active timeline, only incremental should kick in. | ||
| readAndAssert(IncrSourceHelper.MissingCheckpointStrategy.READ_UPTO_LATEST_COMMIT, Option.of("400"), 100, inserts5.getKey()); | ||
|
|
||
| // read just the latest | ||
| readAndAssert(IncrSourceHelper.MissingCheckpointStrategy.READ_LATEST, Option.empty(), 100, inserts5.getKey()); | ||
|
|
||
| // ensure checkpoint does not move | ||
| readAndAssert(IncrSourceHelper.MissingCheckpointStrategy.READ_LATEST, Option.of(inserts5.getKey()), 0, inserts5.getKey()); | ||
|
|
||
| Pair<String, List<HoodieRecord>> inserts6 = writeRecords(writeClient, null, "600"); | ||
|
|
||
| // insert new batch and ensure the checkpoint moves | ||
| readAndAssert(IncrSourceHelper.MissingCheckpointStrategy.READ_LATEST, Option.of(inserts5.getKey()), 100, inserts6.getKey()); | ||
| writeClient.close(); | ||
| } | ||
|
|
||
| private void readAndAssert(IncrSourceHelper.MissingCheckpointStrategy missingCheckpointStrategy, Option<String> checkpointToPull, int expectedCount, String expectedCheckpoint) { | ||
|
|
||
| Properties properties = new Properties(); | ||
| properties.setProperty("hoodie.deltastreamer.source.hoodieincr.path", basePath()); | ||
| properties.setProperty("hoodie.deltastreamer.source.hoodieincr.missing.checkpoint.strategy", missingCheckpointStrategy.name()); | ||
| TypedProperties typedProperties = new TypedProperties(properties); | ||
| S3EventsHoodieIncrSource s3IncrSource = new S3EventsHoodieIncrSource(typedProperties, jsc(), spark(), new DummySchemaProvider(S3EventsSchemaUtils.generateS3EventSchema())); | ||
|
|
||
| // read everything until latest | ||
| Pair<Option<Dataset<Row>>, String> batchCheckPoint = s3IncrSource.fetchNextBatch(checkpointToPull, 500); | ||
| Assertions.assertNotNull(batchCheckPoint.getValue()); | ||
| if (expectedCount == 0) { | ||
| assertFalse(batchCheckPoint.getKey().isPresent()); | ||
| } else { | ||
| assertEquals(batchCheckPoint.getKey().get().count(), expectedCount); | ||
| } | ||
| Assertions.assertEquals(batchCheckPoint.getRight(), expectedCheckpoint); | ||
| } | ||
|
|
||
| private Pair<String, List<HoodieRecord>> writeRecords(SparkRDDWriteClient writeClient, List<HoodieRecord> insertRecords, String commit) throws IOException { | ||
| writeClient.startCommitWithTime(commit); | ||
| List<HoodieRecord> records = dataGen.generateInsertsWithSchema(commit, 100, S3_EVENTS_SCHEMA); | ||
| JavaRDD<WriteStatus> result = writeClient.upsert(jsc().parallelize(records, 1), commit); | ||
| List<WriteStatus> statuses = result.collect(); | ||
| assertNoWriteErrors(statuses); | ||
| return Pair.of(commit, records); | ||
| } | ||
|
|
||
| private HoodieWriteConfig.Builder getConfigBuilder(String basePath, HoodieTableMetaClient metaClient) { | ||
| return HoodieWriteConfig.newBuilder().withPath(basePath).withSchema(S3_EVENTS_SCHEMA) | ||
| .withParallelism(2, 2).withBulkInsertParallelism(2).withFinalizeWriteParallelism(2).withDeleteParallelism(2) | ||
| .withTimelineLayoutVersion(TimelineLayoutVersion.CURR_VERSION) | ||
| .forTable(metaClient.getTableConfig().getTableName()); | ||
| } | ||
|
|
||
| private static class DummySchemaProvider extends SchemaProvider { | ||
|
|
||
| private final Schema schema; | ||
|
|
||
| public DummySchemaProvider(Schema schema) { | ||
| super(new TypedProperties()); | ||
| this.schema = schema; | ||
| } | ||
|
|
||
| @Override | ||
| public Schema getSourceSchema() { | ||
| return schema; | ||
| } | ||
| } | ||
| } | ||
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.
RawTripTestPayloadassumes some form of trips schema. If you look at its constructor, we don't use the schema. And its APIs assume a few things about the schema. Should we keep all this out ofHoodieTestDataGenerator?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.
This is an old comment. Please check if it's still valid.