You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@impala.apache.org by "Peter Rozsa (Code Review)" <ge...@cloudera.org> on 2025/02/06 13:24:15 UTC

[Impala-ASF-CR] IMPALA-13611: Add interop tests for Iceberg tables

Peter Rozsa has uploaded this change for review. ( http://gerrit.cloudera.org:8080/22459


Change subject: IMPALA-13611: Add interop tests for Iceberg tables
......................................................................

IMPALA-13611: Add interop tests for Iceberg tables

This change adds new e2e tests where an Iceberg table is modified from
an external system. These tests validate that externally modified, but
HMS-tracked tables remain synchronized, and both actors see the
same results.

Change-Id: Ic2ee6d3354c3b11264c5e3ded9826831e3962a98
---
M tests/stress/test_update_stress.py
1 file changed, 92 insertions(+), 2 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/59/22459/1
-- 
To view, visit http://gerrit.cloudera.org:8080/22459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic2ee6d3354c3b11264c5e3ded9826831e3962a98
Gerrit-Change-Number: 22459
Gerrit-PatchSet: 1
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>

[Impala-ASF-CR] IMPALA-13611: Add interop tests for Iceberg tables

Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.

Peter Rozsa has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/22459 )

Change subject: IMPALA-13611: Add interop tests for Iceberg tables
......................................................................

IMPALA-13611: Add interop tests for Iceberg tables

This change adds new e2e tests where an Iceberg table is modified from
an external system. These tests validate that externally modified, but
HMS-tracked tables remain synchronized, and both actors see the
same results.

Change-Id: Ic2ee6d3354c3b11264c5e3ded9826831e3962a98
---
M tests/stress/test_update_stress.py
1 file changed, 91 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/59/22459/2
-- 
To view, visit http://gerrit.cloudera.org:8080/22459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic2ee6d3354c3b11264c5e3ded9826831e3962a98
Gerrit-Change-Number: 22459
Gerrit-PatchSet: 2
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>

[Impala-ASF-CR] IMPALA-13611: Add interop tests for Iceberg tables

Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.

Peter Rozsa has posted comments on this change. ( http://gerrit.cloudera.org:8080/22459 )

Change subject: IMPALA-13611: Add interop tests for Iceberg tables
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/22459/1/tests/stress/test_update_stress.py
File tests/stress/test_update_stress.py:

http://gerrit.cloudera.org:8080/#/c/22459/1/tests/stress/test_update_stress.py@226
PS1, Line 226: ve_client.close()
> Yes, Hive updater should be balanced with Impala deleter, my mistake.
I did some experiments with checking by waiting for Hive to do at least one update, but I have the feeling that with this approach I technically did a synchronization, and I think, the core of this test is to do concurrent operations without additional synchronization, just rely on the Iceberg table's concurrency model. Even if it's theoretically possible that Hive's writing is missed, I think the only thing that we should do here is to avoid the direct lock-out of each writer by shifting their timing.



-- 
To view, visit http://gerrit.cloudera.org:8080/22459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic2ee6d3354c3b11264c5e3ded9826831e3962a98
Gerrit-Change-Number: 22459
Gerrit-PatchSet: 2
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Fri, 14 Mar 2025 13:31:00 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-13611: Add interop tests for Iceberg tables

Posted by "Noemi Pap-Takacs (Code Review)" <ge...@cloudera.org>.

Noemi Pap-Takacs has posted comments on this change. ( http://gerrit.cloudera.org:8080/22459 )

Change subject: IMPALA-13611: Add interop tests for Iceberg tables
......................................................................


Patch Set 2: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/22459/1/tests/stress/test_update_stress.py
File tests/stress/test_update_stress.py:

http://gerrit.cloudera.org:8080/#/c/22459/1/tests/stress/test_update_stress.py@226
PS1, Line 226: ve_client.close()
> I did some experiments with checking by waiting for Hive to do at least one
I see, thanks.



-- 
To view, visit http://gerrit.cloudera.org:8080/22459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic2ee6d3354c3b11264c5e3ded9826831e3962a98
Gerrit-Change-Number: 22459
Gerrit-PatchSet: 2
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 18 Mar 2025 15:53:18 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-13611: Add interop tests for Iceberg tables

Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.

Peter Rozsa has posted comments on this change. ( http://gerrit.cloudera.org:8080/22459 )

Change subject: IMPALA-13611: Add interop tests for Iceberg tables
......................................................................


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/22459/1/tests/stress/test_update_stress.py
File tests/stress/test_update_stress.py:

http://gerrit.cloudera.org:8080/#/c/22459/1/tests/stress/test_update_stress.py@215
PS1, Line 215:     """Increments j's value with each iteration until all rows are deleted"""
> This comment doesn't seem to be for this function. Neither 'num_inserts' no
Done


http://gerrit.cloudera.org:8080/#/c/22459/1/tests/stress/test_update_stress.py@226
PS1, Line 226: ve_client.close()
> Did you check if the number if deletes and updates are balanced also within
It's hard to tell what is balanced in this situation. My observation is that if one of the writer's clocks is not shifted a bit, they lock each other from committing. If I keep Hive's interval 7 times higher then Impala's deleted job will do multiple deletes, and Hive can do one update at least. I know it's not optimal, but this case will run for a lot more time without shifting.


http://gerrit.cloudera.org:8080/#/c/22459/1/tests/stress/test_update_stress.py@282
PS1, Line 282: t(
> num_rows
Done


http://gerrit.cloudera.org:8080/#/c/22459/1/tests/stress/test_update_stress.py@358
PS1, Line 358:     """Issues DELETE statements from Impala and UPDATE statements from Hive
> Couldn't the implementations of these two functions be merged, with the del
The whole class could be refactored to make the operators interchangeable, I followed the pattern that is already used here. I think, for tests, simplicity and readability are more important than the code size. I can rewrite it, but in this case, I'd refactor the whole class, and I could not promise a better result at the end.


http://gerrit.cloudera.org:8080/#/c/22459/1/tests/stress/test_update_stress.py@409
PS1, Line 409: 
> Is it intentional that test_iceberg_impala_deletes_and_hive_updates doesn't
Yes, in this case, Hive's deletion must be processed by event processing or by an explicit REFRESH. In this case, I choose REFRESH. The other case will always show the correct result without REFRESH as the last "updater" of the table is Impala.



-- 
To view, visit http://gerrit.cloudera.org:8080/22459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic2ee6d3354c3b11264c5e3ded9826831e3962a98
Gerrit-Change-Number: 22459
Gerrit-PatchSet: 2
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Mon, 24 Feb 2025 17:57:47 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-13611: Add interop tests for Iceberg tables

Posted by "Noemi Pap-Takacs (Code Review)" <ge...@cloudera.org>.

Noemi Pap-Takacs has posted comments on this change. ( http://gerrit.cloudera.org:8080/22459 )

Change subject: IMPALA-13611: Add interop tests for Iceberg tables
......................................................................


Patch Set 1:

(2 comments)

Thanks for adding this test, Peter!

http://gerrit.cloudera.org:8080/#/c/22459/1/tests/stress/test_update_stress.py
File tests/stress/test_update_stress.py:

http://gerrit.cloudera.org:8080/#/c/22459/1/tests/stress/test_update_stress.py@226
PS1, Line 226: time.sleep(random.random() * 7)
Did you check if the number if deletes and updates are balanced also within and between the engines?

Does this multiplier mean that Hive updates are generally 7x faster than Hive deletes and Impala updates?


http://gerrit.cloudera.org:8080/#/c/22459/1/tests/stress/test_update_stress.py@282
PS1, Line 282: 10
num_rows



-- 
To view, visit http://gerrit.cloudera.org:8080/22459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic2ee6d3354c3b11264c5e3ded9826831e3962a98
Gerrit-Change-Number: 22459
Gerrit-PatchSet: 1
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Mon, 24 Feb 2025 14:49:04 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-13611: Add interop tests for Iceberg tables

Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.

Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/22459 )

Change subject: IMPALA-13611: Add interop tests for Iceberg tables
......................................................................


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/22459/1/tests/stress/test_update_stress.py
File tests/stress/test_update_stress.py:

http://gerrit.cloudera.org:8080/#/c/22459/1/tests/stress/test_update_stress.py@215
PS1, Line 215:     """Writes ascending numbers up to 'num_inserts' into column 'i'. To column 'wid' it
This comment doesn't seem to be for this function. Neither 'num_inserts' nor 'wid' exist.


http://gerrit.cloudera.org:8080/#/c/22459/1/tests/stress/test_update_stress.py@358
PS1, Line 358:   def test_iceberg_impala_deletes_and_hive_updates(self, unique_database):
Couldn't the implementations of these two functions be merged, with the deleter, updater and checker passed in as paremeters?


http://gerrit.cloudera.org:8080/#/c/22459/1/tests/stress/test_update_stress.py@409
PS1, Line 409:     self.client.execute("refresh %s" % tbl_name)
Is it intentional that test_iceberg_impala_deletes_and_hive_updates doesn't call REFRESH?



-- 
To view, visit http://gerrit.cloudera.org:8080/22459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic2ee6d3354c3b11264c5e3ded9826831e3962a98
Gerrit-Change-Number: 22459
Gerrit-PatchSet: 1
Gerrit-Owner: Peter Rozsa <pr...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Mon, 24 Feb 2025 15:27:24 +0000
Gerrit-HasComments: Yes