You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by GitBox <gi...@apache.org> on 2022/09/08 17:40:24 UTC
[GitHub] [hudi] rahil-c opened a new pull request, #6637: Fix AWSDmsAvroPayload#getInsertValue,combineAndGetUpdateValue to invo…
rahil-c opened a new pull request, #6637:
URL: https://github.com/apache/hudi/pull/6637
…ke correct api
### Change Logs
_Describe context and summary for this change. Highlight if any code was copied._
### Impact
_Describe any public API or user-facing feature change or any performance impact._
**Risk level: none | low | medium | high**
_Choose one. If medium or high, explain what verification was done to mitigate the risks._
### Contributor's checklist
- [ ] Read through [contributor's guide](https://hudi.apache.org/contribute/how-to-contribute)
- [ ] Change Logs and Impact were stated clearly
- [ ] Adequate tests were added if applicable
- [ ] CI passed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hudi] rahil-c commented on pull request #6637: Fix AWSDmsAvroPayload#getInsertValue,combineAndGetUpdateValue to invo…
Posted by GitBox <gi...@apache.org>.
rahil-c commented on PR #6637:
URL: https://github.com/apache/hudi/pull/6637#issuecomment-1242539909
Had offline sync with @umehrot2 to clarify above dicussion, and have amended `TestAWSDmsAvroPayload` in new rev
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hudi] hudi-bot commented on pull request #6637: Fix AWSDmsAvroPayload#getInsertValue,combineAndGetUpdateValue to invo…
Posted by GitBox <gi...@apache.org>.
hudi-bot commented on PR #6637:
URL: https://github.com/apache/hudi/pull/6637#issuecomment-1241264591
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "5b7d712d175b64de73ce924bbdb95962ebb790fe",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=11256",
"triggerID" : "5b7d712d175b64de73ce924bbdb95962ebb790fe",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 5b7d712d175b64de73ce924bbdb95962ebb790fe Azure: [FAILURE](https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=11256)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hudi] hudi-bot commented on pull request #6637: Fix AWSDmsAvroPayload#getInsertValue,combineAndGetUpdateValue to invo…
Posted by GitBox <gi...@apache.org>.
hudi-bot commented on PR #6637:
URL: https://github.com/apache/hudi/pull/6637#issuecomment-1242550582
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "5b7d712d175b64de73ce924bbdb95962ebb790fe",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=11256",
"triggerID" : "5b7d712d175b64de73ce924bbdb95962ebb790fe",
"triggerType" : "PUSH"
}, {
"hash" : "1d917c00cbb96a970c0f7bdf3d26d4af9a9dd357",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=11279",
"triggerID" : "1d917c00cbb96a970c0f7bdf3d26d4af9a9dd357",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 5b7d712d175b64de73ce924bbdb95962ebb790fe Azure: [FAILURE](https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=11256)
* 1d917c00cbb96a970c0f7bdf3d26d4af9a9dd357 Azure: [PENDING](https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=11279)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hudi] hudi-bot commented on pull request #6637: Fix AWSDmsAvroPayload#getInsertValue,combineAndGetUpdateValue to invo…
Posted by GitBox <gi...@apache.org>.
hudi-bot commented on PR #6637:
URL: https://github.com/apache/hudi/pull/6637#issuecomment-1242589047
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "5b7d712d175b64de73ce924bbdb95962ebb790fe",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=11256",
"triggerID" : "5b7d712d175b64de73ce924bbdb95962ebb790fe",
"triggerType" : "PUSH"
}, {
"hash" : "1d917c00cbb96a970c0f7bdf3d26d4af9a9dd357",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=11279",
"triggerID" : "1d917c00cbb96a970c0f7bdf3d26d4af9a9dd357",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 1d917c00cbb96a970c0f7bdf3d26d4af9a9dd357 Azure: [SUCCESS](https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=11279)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hudi] hudi-bot commented on pull request #6637: Fix AWSDmsAvroPayload#getInsertValue,combineAndGetUpdateValue to invo…
Posted by GitBox <gi...@apache.org>.
hudi-bot commented on PR #6637:
URL: https://github.com/apache/hudi/pull/6637#issuecomment-1241064229
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "5b7d712d175b64de73ce924bbdb95962ebb790fe",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=11256",
"triggerID" : "5b7d712d175b64de73ce924bbdb95962ebb790fe",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 5b7d712d175b64de73ce924bbdb95962ebb790fe Azure: [PENDING](https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=11256)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hudi] rahil-c commented on pull request #6637: Fix AWSDmsAvroPayload#getInsertValue,combineAndGetUpdateValue to invo…
Posted by GitBox <gi...@apache.org>.
rahil-c commented on PR #6637:
URL: https://github.com/apache/hudi/pull/6637#issuecomment-1242384390
> Fix LGTM. However, we should not be adding this whole end to end test in `TestCOWDataSource` and `TestMORDataSource`. These tests are there to test overall datasource related functionality, and should not really be used to test something so specific as DMS payload. There should be no need to run an entire end to end test to discover this issue.
>
> There is a `TestAWSDmsAvroPayload` class. We should understand why tests in that class did not catch the issue, and just modify them or add a new test as needed to be able to catch this issue.
Sounds good I agree that adding this unittest in this class might be unnecessary. As for modifying or adding a new test within `TestAWSDmsAvroPayload` this also might be not needed as the tests actually are referring to the correct api and do have coverage. For example in one of the unit tests of that class that handles delete case
```
@Test
public void testDelete() {
Schema avroSchema = new Schema.Parser().parse(AVRO_SCHEMA_STRING);
GenericRecord deleteRecord = new GenericData.Record(avroSchema);
deleteRecord.put("field1", 2);
deleteRecord.put("Op", "D");
GenericRecord oldRecord = new GenericData.Record(avroSchema);
oldRecord.put("field1", 2);
oldRecord.put("Op", "U");
AWSDmsAvroPayload payload = new AWSDmsAvroPayload(Option.of(deleteRecord));
try {
Option<IndexedRecord> outputPayload = payload.combineAndGetUpdateValue(oldRecord, avroSchema);
// expect nothing to be committed to table
assertFalse(outputPayload.isPresent());
} catch (Exception e) {
fail("Unexpected exception");
}
}
```
it uses the correct `combineAndGetUpdateValue(oldRecord, avroSchema);` method signature which is not requiring us to take in account the `properties` object.
If we want to reproduce the same exception as what was reported we can add the `properties` object to the call ` Option<IndexedRecord> outputPayload = payload.combineAndGetUpdateValue(oldRecord, avroSchema, properties);` and get the same exception
```
ava.util.NoSuchElementException: No value present in Option
at org.apache.hudi.common.util.Option.get(Option.java:89)
at org.apache.hudi.common.model.AWSDmsAvroPayload.combineAndGetUpdateValue(AWSDmsAvroPayload.java:85)
at org.apache.hudi.common.model.TestAWSDmsAvroPayload.testDeleteWithProperties(TestAWSDmsAvroPayload.java:123)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
```
But this however would not make sense as this code change is making sure to use the signature without the `properties` for both `return getInsertValue(schema)` and `
return combineAndGetUpdateValue(currentValue, schema)` similar to what the tests have been doing for some time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hudi] umehrot2 commented on pull request #6637: Fix AWSDmsAvroPayload#getInsertValue,combineAndGetUpdateValue to invo…
Posted by GitBox <gi...@apache.org>.
umehrot2 commented on PR #6637:
URL: https://github.com/apache/hudi/pull/6637#issuecomment-1241218981
Fix LGTM.
However, we should not be adding this whole end to end test in `TestCOWDataSource` and `TestMORDataSource`. These tests are there to test overall datasource related functionality, and should not really be used to test something so specific as DMS payload. There should be no need to run an entire end to end test to discover this issue.
There is a `TestAWSDmsAvroPayload` class. We should understand why tests in that class did not catch the issue, and just modify them or add a new test as needed to be able to catch this issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hudi] yihua merged pull request #6637: [HUDI-4831] Fix AWSDmsAvroPayload#getInsertValue,combineAndGetUpdateValue to invoked correct api
Posted by GitBox <gi...@apache.org>.
yihua merged PR #6637:
URL: https://github.com/apache/hudi/pull/6637
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hudi] hudi-bot commented on pull request #6637: Fix AWSDmsAvroPayload#getInsertValue,combineAndGetUpdateValue to invo…
Posted by GitBox <gi...@apache.org>.
hudi-bot commented on PR #6637:
URL: https://github.com/apache/hudi/pull/6637#issuecomment-1241058661
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "5b7d712d175b64de73ce924bbdb95962ebb790fe",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "5b7d712d175b64de73ce924bbdb95962ebb790fe",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 5b7d712d175b64de73ce924bbdb95962ebb790fe UNKNOWN
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hudi] hudi-bot commented on pull request #6637: Fix AWSDmsAvroPayload#getInsertValue,combineAndGetUpdateValue to invo…
Posted by GitBox <gi...@apache.org>.
hudi-bot commented on PR #6637:
URL: https://github.com/apache/hudi/pull/6637#issuecomment-1242548287
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "5b7d712d175b64de73ce924bbdb95962ebb790fe",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=11256",
"triggerID" : "5b7d712d175b64de73ce924bbdb95962ebb790fe",
"triggerType" : "PUSH"
}, {
"hash" : "1d917c00cbb96a970c0f7bdf3d26d4af9a9dd357",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "1d917c00cbb96a970c0f7bdf3d26d4af9a9dd357",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 5b7d712d175b64de73ce924bbdb95962ebb790fe Azure: [FAILURE](https://dev.azure.com/apache-hudi-ci-org/785b6ef4-2f42-4a89-8f0e-5f0d7039a0cc/_build/results?buildId=11256)
* 1d917c00cbb96a970c0f7bdf3d26d4af9a9dd357 UNKNOWN
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org