Hoodie Event callbacks #251

kaushikd49 · 2017-08-22T22:54:34Z

Addresses #153 (Implement Hoodie Event callbacks)

kaushikd49 · 2017-08-22T23:43:53Z

CI failure is due to the race condition bug on master. This PR has been successfully tested on master with the patch given by @n3nash to avoid race temporarily. PR has has also been tested with an ingestion job.

prazanna · 2017-09-04T06:22:05Z

hoodie-client/src/main/java/com/uber/hoodie/config/HoodieWriteConfig.java

@@ -102,6 +105,17 @@ public StorageLevel getWriteStatusStorageLevel() {
        return StorageLevel.fromString(props.getProperty(WRITE_STATUS_STORAGE_LEVEL));
    }

+    public <T extends WriteStatus> T getWriteStatusInstance() {
+        try {
+            Class<? extends WriteStatus> aClass = (Class<? extends WriteStatus>) Class


Use ReflectionUtils to cache the class instance and config is not the right place to instantiate the class. I would do it as a interface method (factory) in WriteStatus interface

Sure Prasanna, following is what you mean right?

In HoodieWriteConfig add below

public WriteStatus getWriteStatus() { return ReflectionUtils.loadClass(props.getProperty(HOODIE_WRITE_STATUS_CLASS_PROP)); }

I agree with what prasanna is saying. We can't be actually instantiating the class from the config class. i.e the method here can just return you the class name..

Please move the actual creation of the class i.e the ReflectionUtils call into the Handle classes themselves.

prazanna · 2017-09-04T06:27:55Z

hoodie-client/src/main/java/com/uber/hoodie/WriteStatus.java

@@ -47,12 +48,14 @@
    private long totalRecords = 0;
    private long totalErrorRecords = 0;

-    public void markSuccess(HoodieRecord record) {
+    public void markSuccess(HoodieRecord record,
+        Map<String, String> recordMetadata) {


Make WriteStatus a interface and have a Default implementation

Sure, Im planning to have the default implementation also not use recordMetadata. Please let me know if you prefer having the recordMetadata merging logic in the default impl too.

prazanna · 2017-09-04T06:32:51Z

hoodie-common/src/main/java/com/uber/hoodie/common/model/HoodieRecordPayload.java

+     * This method can be used to fetch metadata regarding the HoodieRecordPayload, for the purpose
+     * of capturing some metrics.
+     */
+    Map<String, String> getMetadata();


I am a little concerned with having a map of properties on the record payload. Adding data to record payload is very tricky in terms of shuffle. It put in a 100 properties and tuned it for 10 - things may fail miserably during a shuffle. If we need to encode data - why dont we just do it in the InsertRecord. Can you may be give me an example where storing it in the record itself would not make sense?

CMIIW, even if we encode it in the InsertRecord (or HoodieRecordPayload), we need this method to fetch the metadata just before HoodieRecordPayload is deflated like in 8f2c88b#diff-165908ecbfe7dbae538c00e8157e7be0R129

Regarding storing properties in a map - did you recommend storing it in the IndexedRecord instead? My concern was inadvertent stuff being put into IndexRecord affecting parquet schema. But we are planning to do that anyway for across_batch_de-duped counts, so yes, I could actually move these properties into IndexedRecord itself.

IIUC you want a mechanism to pass some data from each record and have it propagated down to a custom WriteStatus class? Since it was mostly you and @prazanna who discussed this and neither the ticket nor the PR describes what the commit is about, @kaushikd49 could you please summarize what the real use-case is ?

I am okay with this approach @kaushikd49. We may want to enable adding properties to HoodieRecordPayload with a property in HoodieWriteConfig (disabled by default). It may be good idea to write up a small doc on how you are planning to use this map downstream internally and share it with @vinothchandar - just so he has context. Thanks and sorry for the delay

@vinothchandar Yea that is right. I will be sharing a doc with you shortly explaining the use-case.

@prazanna Sure, I'll do that.

kaushikd49 · 2017-09-18T18:48:55Z

@prazanna Thanks a lot for your comments. Had few questions on the comments. Could you please help answer them?

vinothchandar · 2017-09-25T19:53:09Z

@kaushikd49 I will take a look and in the meantime, can you please rebase this again off master

kaushikd49 · 2017-09-25T20:02:57Z

Thanks, done @vinothchandar

vinothchandar · 2017-09-26T18:08:48Z

hoodie-client/src/test/java/com/uber/hoodie/common/TestRawTripPayload.java

@@ -91,6 +97,14 @@ public String getPartitionPath() {
        }
    }

+    @Override
+    public Map<String, String> getMetadata() {
+        Map<String, String> metadataMap = new HashMap<>();


Can you come up with some reasonable values here for the map? Current values don't convey your usage pattern well IMO

vinothchandar · 2017-09-26T18:12:03Z

hoodie-client/src/test/java/com/uber/hoodie/table/TestCopyOnWriteTable.java

@@ -104,7 +105,7 @@ private HoodieWriteConfig makeHoodieClientConfig() throws Exception {
    private HoodieWriteConfig.Builder makeHoodieClientConfigBuilder() throws Exception {
        // Prepare the AvroParquetIO
        String schemaStr = IOUtils.toString(getClass().getResourceAsStream("/exampleSchema.txt"), "UTF-8");
-        return HoodieWriteConfig.newBuilder().withPath(basePath).withSchema(schemaStr);
+        return HoodieWriteConfig.newBuilder().withPath(basePath).withSchema(schemaStr).withWriteStatusClass(SampleMergeMetadataWriteStatus.class);


Please write a separate test case, which demonstrates how the framework should be used. Lets not change the existing tests with a custom WriteStatus class, as I want to ensure the default usage with WriteStatus.class continues to be exercised.

vinothchandar · 2017-09-26T18:13:25Z

hoodie-common/src/main/java/com/uber/hoodie/common/HoodieJsonPayload.java

@@ -57,6 +59,11 @@ public HoodieJsonPayload(String json) throws IOException {
        return Optional.of(jsonConverter.convert(getJsonData()));
    }

+    @Override


Can you push this to the base class, instead of implementing in each subclass

vinothchandar · 2017-09-26T18:17:56Z

hoodie-common/src/main/java/com/uber/hoodie/common/model/HoodieRecordPayload.java

+     * This method can be used to fetch metadata regarding the HoodieRecordPayload, for the purpose
+     * of capturing some metrics.
+     */
+    Map<String, String> getMetadata();


In fact, can you make this an Optional<Map<String, String>> and always return Optional.empty from the base class?

vinothchandar

Love some clarification on actual use-case
Don't have actual instantiation in the HoodieWriteConfig class
API change & clear documentation for getMetadata, and also markSuccess/markFailure methods to explain what is passed in, warnings about having a large map in each record etc..
Leaving existing tests working on WriteStatus
Write a separate test case (not a file, just a case) which clearly demonstrates your use-case

vinothchandar · 2017-09-26T18:21:57Z

hoodie-client/src/main/java/com/uber/hoodie/WriteStatus.java

@@ -47,12 +48,14 @@
    private long totalRecords = 0;
    private long totalErrorRecords = 0;

-    public void markSuccess(HoodieRecord record) {
+    public void markSuccess(HoodieRecord record,


javadocs for parameters..

kaushikd49 · 2017-09-26T21:35:23Z

@vinothchandar @prazanna Thank you, will send out an updated PR

kaushikd49 · 2017-09-29T06:14:17Z

@vinothchandar @prazanna Addressed your comments.

We may want to enable adding properties to HoodieRecordPayload with a property in HoodieWriteConfig (disabled by default)

Did not add this property because in the default WriteStatus implementation class (WriteStatusImpl), the metadata map is anyway not used. Also, this adds a dependency to HoodieWriteConfig class from WriteStatus Interface. Open to your suggestions on this.

vinothchandar · 2017-09-29T17:27:05Z

hoodie-client/src/main/java/com/uber/hoodie/io/HoodieAppendHandle.java

@@ -74,7 +73,7 @@ public HoodieAppendHandle(HoodieWriteConfig config,
                              String fileId,
                              Iterator<HoodieRecord<T>> recordItr) {
        super(config, commitTime, hoodieTable);
-        WriteStatus writeStatus = new WriteStatus();
+        WriteStatus writeStatus = ReflectionUtils.loadClass(config.getWriteStatusClassName());


If you re going to rely on a constructor being called via Reflection, then making WriteStatus an interface is not a necessary thing.. Atleast in the subclassing route, you will be forcing the subclass to implement atleast 1 constructor of the same signature as base class

Was this part of @prazanna 's suggestion earlier on? (given then comment history is nt there now)

@vinothchandar Yea, it was part of @prazanna's suggestion earlier. The rebase has hidden the comment. Ok, I'll retain WriteStatus like before as a class.

vinothchandar · 2017-09-29T17:27:47Z

hoodie-client/src/test/java/com/uber/hoodie/TestMergeOnReadTable.java

+        HoodieWriteConfig cfg = getConfigBuilder().withWriteStatusClass(MetadataMergeWriteStatus.class).build();
+        HoodieWriteClient client = new HoodieWriteClient(jsc, cfg);
+
+        /**


there is only 1 write.. please remove comment block.

vinothchandar · 2017-09-29T17:30:47Z

Generally okay with the change. 1 question on interfacing vs subclassing. Also please ensure you rebased against latest master, since I changed WriteStatus yest

kaushikd49 · 2017-09-29T21:24:29Z

@vinothchandar Done. Yea it is rebased with latest master.

vinothchandar · 2017-09-29T21:34:20Z

hoodie-client/src/main/java/com/uber/hoodie/WriteStatus.java


 /**
 * Status of a write operation.
 */
 public class WriteStatus implements Serializable {

-    private final HashMap<HoodieKey, Throwable> errors = new HashMap<>();
+  private final HashMap<HoodieKey, Throwable> errors = new HashMap<>();


Indenting has changed here throughout.. Can you reformat this file and ensure the only diffs we see here are the two methods? https://uber.github.io/hoodie/code_structure.html

Sorry about that.

Im using the same code style, and it seems like it is using +2 spaces for indentation. https://google.github.io/styleguide/javaguide.html#s4.2-block-indentation

vinothchandar · 2017-09-29T21:37:06Z

hoodie-client/src/test/java/com/uber/hoodie/table/TestCopyOnWriteTable.java

@@ -238,7 +239,40 @@ public void testUpdateRecords() throws Exception {
        return records;
    }

-    @Test public void testInsertWithPartialFailures() throws Exception {
+    // Check if record level metadata is aggregated properly at the end of write.


watch for indentation here too

vinothchandar · 2017-09-29T21:37:52Z

hoodie-common/src/main/java/com/uber/hoodie/common/HoodieJsonPayload.java

@@ -20,6 +20,8 @@
 import com.uber.hoodie.common.model.HoodieRecordPayload;
 import com.uber.hoodie.exception.HoodieException;

+import java.util.Collections;


extra changes that are not needed?

vinothchandar

Cosmetic changes. Should be ready to merge otherwise

kaushikd49 · 2017-09-29T22:21:05Z

@vinothchandar Done.

kaushikd49 force-pushed the record_counts_hook branch from 311522a to 35d62fe Compare August 22, 2017 22:55

kaushikd49 changed the title ~~Record counts hook~~ Hoodie Event callbacks Aug 22, 2017

kaushikd49 force-pushed the record_counts_hook branch from 35d62fe to 8f2c88b Compare August 22, 2017 23:23

prazanna suggested changes Sep 4, 2017

View reviewed changes

vinothchandar self-requested a review September 25, 2017 19:53

kaushikd49 force-pushed the record_counts_hook branch from 8f2c88b to 66312b7 Compare September 25, 2017 19:59

vinothchandar reviewed Sep 26, 2017

View reviewed changes

vinothchandar requested changes Sep 26, 2017

View reviewed changes

vinothchandar reviewed Sep 26, 2017

View reviewed changes

kaushikd49 force-pushed the record_counts_hook branch 2 times, most recently from 073807f to 38c134a Compare September 29, 2017 06:00

vinothchandar reviewed Sep 29, 2017

View reviewed changes

kaushikd49 force-pushed the record_counts_hook branch from 38c134a to 0ac219f Compare September 29, 2017 20:55

vinothchandar reviewed Sep 29, 2017

View reviewed changes

capture record metadata before deflating for record counting

40b1701

kaushikd49 force-pushed the record_counts_hook branch from 0ac219f to 40b1701 Compare September 29, 2017 22:05

vinothchandar approved these changes Oct 2, 2017

View reviewed changes

vinothchandar merged commit c98ee05 into apache:master Oct 2, 2017

vinishjail97 pushed a commit to vinishjail97/hudi that referenced this pull request Dec 15, 2023

[ONHS-7165] Json schema value deserializer changes (apache#251)

4349b9d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hoodie Event callbacks #251

Hoodie Event callbacks #251

kaushikd49 commented Aug 22, 2017

kaushikd49 commented Aug 22, 2017

prazanna Sep 4, 2017

kaushikd49 Sep 13, 2017 •

edited

vinothchandar Sep 26, 2017

prazanna Sep 4, 2017

kaushikd49 Sep 13, 2017 •

edited

prazanna Sep 4, 2017

kaushikd49 Sep 13, 2017

vinothchandar Sep 26, 2017

prazanna Sep 26, 2017

kaushikd49 Sep 26, 2017

kaushikd49 commented Sep 18, 2017

vinothchandar commented Sep 25, 2017

kaushikd49 commented Sep 25, 2017

vinothchandar Sep 26, 2017

vinothchandar Sep 26, 2017

vinothchandar Sep 26, 2017

vinothchandar Sep 26, 2017

vinothchandar left a comment

vinothchandar Sep 26, 2017

kaushikd49 commented Sep 26, 2017

kaushikd49 commented Sep 29, 2017

vinothchandar Sep 29, 2017

kaushikd49 Sep 29, 2017

vinothchandar Sep 29, 2017

vinothchandar commented Sep 29, 2017

kaushikd49 commented Sep 29, 2017 •

edited

vinothchandar Sep 29, 2017

kaushikd49 Sep 29, 2017

vinothchandar Sep 29, 2017

vinothchandar Sep 29, 2017

vinothchandar left a comment

kaushikd49 commented Sep 29, 2017

Hoodie Event callbacks #251

Hoodie Event callbacks #251

Conversation

kaushikd49 commented Aug 22, 2017

kaushikd49 commented Aug 22, 2017

Choose a reason for hiding this comment

kaushikd49 Sep 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaushikd49 Sep 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaushikd49 commented Sep 18, 2017

vinothchandar commented Sep 25, 2017

kaushikd49 commented Sep 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinothchandar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaushikd49 commented Sep 26, 2017

kaushikd49 commented Sep 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinothchandar commented Sep 29, 2017

kaushikd49 commented Sep 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinothchandar left a comment

Choose a reason for hiding this comment

kaushikd49 commented Sep 29, 2017

kaushikd49 Sep 13, 2017 •

edited

kaushikd49 Sep 13, 2017 •

edited

kaushikd49 commented Sep 29, 2017 •

edited