You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org> on 2023/07/23 23:12:50 UTC
[Impala-ASF-CR] IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
Wenzhe Zhou has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20258
Change subject: IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
......................................................................
IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
In IMPALA-12286, catalogd re-generate its Catalog Service ID in
JniCatalog when it becomes active. But CatalogServiceCatalog is not
updated when new Catalog Service ID is generated. This causes
coordinator hanging when processing DDLs.
In CatalogServer class, is_active_ is not protected by mutex
catalog_lock_, and pending_topic_updates_ is not cleared when the
catalogd becomes active. It's possible catalog server sends pending
catalog topic updates with old Catalog Service ID then sends catalog
topic updates with new Catalog Service ID.
This patch sets the Catalog Service ID for CatalogServiceCatalog when
new Catalog Service ID is generated. It makes is_active_ to be protected
by mutex catalog_lock_, and makes catalog server to clear
pending_topic_updates_ when the catalogd becomes active.
Testing:
- Added more queries in test cases for CatalogD HA. Verified that
test cases failed without fix, and test cases were passed after fix.
- Passed core tests.
Change-Id: I5eada89052c5f16209c6f16357f30f78b4497434
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-server.h
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M tests/custom_cluster/test_catalogd_ha.py
5 files changed, 94 insertions(+), 58 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/58/20258/1
--
To view, visit http://gerrit.cloudera.org:8080/20258
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5eada89052c5f16209c6f16357f30f78b4497434
Gerrit-Change-Number: 20258
Gerrit-PatchSet: 1
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
[Impala-ASF-CR] IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/20258 )
Change subject: IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20258/2/tests/custom_cluster/test_catalogd_ha.py
File tests/custom_cluster/test_catalogd_ha.py:
http://gerrit.cloudera.org:8080/#/c/20258/2/tests/custom_cluster/test_catalogd_ha.py@70
PS2, Line 70: self.client, "drop table if exists test_catalogd_ha")
> nit: maybe it is better for test calling this method to operate with unique
unique_database does not work if impala daemons are restarted during testing. In these test cases, coordinator, catalogd and statestored are restarted so we cannot use it.
--
To view, visit http://gerrit.cloudera.org:8080/20258
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eada89052c5f16209c6f16357f30f78b4497434
Gerrit-Change-Number: 20258
Gerrit-PatchSet: 2
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 24 Jul 2023 19:14:38 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/20258 )
Change subject: IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20258/2/be/src/catalog/catalog-server.cc
File be/src/catalog/catalog-server.cc:
http://gerrit.cloudera.org:8080/#/c/20258/2/be/src/catalog/catalog-server.cc@418
PS2, Line 418: if (IsActive()) {
: lock_guard<mutex> l(catalog_lock_);
> Is there any potential race here? IsActive() obtain catalog_lock_ inside it
Good suggestion. changed the code as suggested. Thanks
--
To view, visit http://gerrit.cloudera.org:8080/20258
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eada89052c5f16209c6f16357f30f78b4497434
Gerrit-Change-Number: 20258
Gerrit-PatchSet: 2
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 24 Jul 2023 20:36:08 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20258 )
Change subject: IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
......................................................................
Patch Set 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9529/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/20258
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eada89052c5f16209c6f16357f30f78b4497434
Gerrit-Change-Number: 20258
Gerrit-PatchSet: 3
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 24 Jul 2023 21:24:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/20258 )
Change subject: IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
......................................................................
IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
In IMPALA-12286, catalogd re-generate its Catalog Service ID in
JniCatalog when it becomes active. But CatalogServiceCatalog is not
updated when new Catalog Service ID is generated. This causes
coordinator hanging when processing DDLs.
In CatalogServer class, is_active_ is not protected by mutex
catalog_lock_, and pending_topic_updates_ is not cleared when the
catalogd becomes active. It's possible catalog server sends pending
catalog topic updates with old Catalog Service ID then sends catalog
topic updates with new Catalog Service ID.
This patch removes catalogServiceId_ from CatalogServiceCatalog, and
makes CatalogServiceCatalog to call JniCatalog.getServiceId() directly.
It makes is_active_ to be protected by mutex catalog_lock_, and makes
catalog server to clear pending_topic_updates_ when the catalogd becomes
active.
Testing:
- Added more queries in test cases for CatalogD HA. Verified that
test cases failed without fix, and test cases were passed after fix.
- Passed core tests.
Change-Id: I5eada89052c5f16209c6f16357f30f78b4497434
Reviewed-on: http://gerrit.cloudera.org:8080/20258
Reviewed-by: Andrew Sherman <as...@cloudera.com>
Reviewed-by: Riza Suminto <ri...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-server.h
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
M fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java
M tests/custom_cluster/test_catalogd_ha.py
7 files changed, 100 insertions(+), 84 deletions(-)
Approvals:
Andrew Sherman: Looks good to me, but someone else must approve
Riza Suminto: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/20258
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I5eada89052c5f16209c6f16357f30f78b4497434
Gerrit-Change-Number: 20258
Gerrit-PatchSet: 4
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[Impala-ASF-CR] IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/20258 )
Change subject: IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
......................................................................
Patch Set 3: Code-Review+2
(1 comment)
Thank you for the answers. The rest looks good to me.
http://gerrit.cloudera.org:8080/#/c/20258/3/be/src/catalog/catalog-server.cc
File be/src/catalog/catalog-server.cc:
http://gerrit.cloudera.org:8080/#/c/20258/3/be/src/catalog/catalog-server.cc@410
PS3, Line 410: UpdateRegisteredCatalogd(active_catalogd_registration);
: if (FLAGS_force_catalogd_active && !IsActive()) {
> If the two catalogd start with FLAGS_force_catalogd_active as true, statest
Ack
--
To view, visit http://gerrit.cloudera.org:8080/20258
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eada89052c5f16209c6f16357f30f78b4497434
Gerrit-Change-Number: 20258
Gerrit-PatchSet: 3
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 24 Jul 2023 22:10:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20258 )
Change subject: IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/13622/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20258
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eada89052c5f16209c6f16357f30f78b4497434
Gerrit-Change-Number: 20258
Gerrit-PatchSet: 2
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 24 Jul 2023 06:53:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/20258 )
Change subject: IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20258/2/be/src/catalog/catalog-server.cc
File be/src/catalog/catalog-server.cc:
http://gerrit.cloudera.org:8080/#/c/20258/2/be/src/catalog/catalog-server.cc@418
PS2, Line 418: if (IsActive()) {
: lock_guard<mutex> l(catalog_lock_);
Is there any potential race here? IsActive() obtain catalog_lock_ inside it, release on method return, and retrieve it again here? Why not simply:
{
lock_guard<mutex> l(catalog_lock_);
if (is_active_) catalog_update_cv_.NotifyOne();
}
--
To view, visit http://gerrit.cloudera.org:8080/20258
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eada89052c5f16209c6f16357f30f78b4497434
Gerrit-Change-Number: 20258
Gerrit-PatchSet: 2
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 24 Jul 2023 19:51:17 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/20258 )
Change subject: IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
......................................................................
Patch Set 2: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/20258
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eada89052c5f16209c6f16357f30f78b4497434
Gerrit-Change-Number: 20258
Gerrit-PatchSet: 2
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 24 Jul 2023 13:56:13 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/20258 )
Change subject: IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
......................................................................
Patch Set 2:
(1 comment)
I have 1 nit comment for the test.
http://gerrit.cloudera.org:8080/#/c/20258/2/tests/custom_cluster/test_catalogd_ha.py
File tests/custom_cluster/test_catalogd_ha.py:
http://gerrit.cloudera.org:8080/#/c/20258/2/tests/custom_cluster/test_catalogd_ha.py@70
PS2, Line 70: self.client, "drop table if exists test_catalogd_ha")
nit: maybe it is better for test calling this method to operate with unique_database? Thus, no need for manual drop.
--
To view, visit http://gerrit.cloudera.org:8080/20258
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eada89052c5f16209c6f16357f30f78b4497434
Gerrit-Change-Number: 20258
Gerrit-PatchSet: 2
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 24 Jul 2023 17:29:41 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20258 )
Change subject: IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
......................................................................
Patch Set 3: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/20258
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eada89052c5f16209c6f16357f30f78b4497434
Gerrit-Change-Number: 20258
Gerrit-PatchSet: 3
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Tue, 25 Jul 2023 01:38:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20258 )
Change subject: IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/13629/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20258
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eada89052c5f16209c6f16357f30f78b4497434
Gerrit-Change-Number: 20258
Gerrit-PatchSet: 3
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 24 Jul 2023 21:03:25 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/20258 )
Change subject: IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
......................................................................
Patch Set 3: Code-Review+1
I read through quickly and looks good
--
To view, visit http://gerrit.cloudera.org:8080/20258
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eada89052c5f16209c6f16357f30f78b4497434
Gerrit-Change-Number: 20258
Gerrit-PatchSet: 3
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 24 Jul 2023 20:43:07 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/20258 )
Change subject: IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20258/3/be/src/catalog/catalog-server.cc
File be/src/catalog/catalog-server.cc:
http://gerrit.cloudera.org:8080/#/c/20258/3/be/src/catalog/catalog-server.cc@410
PS3, Line 410: UpdateRegisteredCatalogd(active_catalogd_registration);
: if (FLAGS_force_catalogd_active && !IsActive()) {
> I'm still not sure about this line. Can you explain what happen if two Cata
If the two catalogd start with FLAGS_force_catalogd_active as true, statestore assign the first one as active, then assign the second one as active. The second one overwrite the first one. There is only one as active. FLAGS_force_catalogd_active should be set by central admin like CM for manual failover, we don't expect both catalogd are started with this flag as true in same time. It's possible to have two concurrent UpdateRegisteredCatalogd calls. We have "sending_sequence_" defined in StatestoreCatalogdMgr to track version of active catalogd. But it's not returned in the registration response. I can address this issue in following patch. FLAGS_force_catalogd_active is not used in R36.
UpdateRegisteredCatalogd() is also called from notification callback. I don't like to return is_final. But we can pass a variable pointer for is_active. If the pointer is not null, return the current active status.
--
To view, visit http://gerrit.cloudera.org:8080/20258
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eada89052c5f16209c6f16357f30f78b4497434
Gerrit-Change-Number: 20258
Gerrit-PatchSet: 3
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 24 Jul 2023 22:06:41 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/20258 )
Change subject: IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
......................................................................
IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
In IMPALA-12286, catalogd re-generate its Catalog Service ID in
JniCatalog when it becomes active. But CatalogServiceCatalog is not
updated when new Catalog Service ID is generated. This causes
coordinator hanging when processing DDLs.
In CatalogServer class, is_active_ is not protected by mutex
catalog_lock_, and pending_topic_updates_ is not cleared when the
catalogd becomes active. It's possible catalog server sends pending
catalog topic updates with old Catalog Service ID then sends catalog
topic updates with new Catalog Service ID.
This patch removes catalogServiceId_ from CatalogServiceCatalog, and
makes CatalogServiceCatalog to call JniCatalog.getServiceId() directly.
It makes is_active_ to be protected by mutex catalog_lock_, and makes
catalog server to clear pending_topic_updates_ when the catalogd becomes
active.
Testing:
- Added more queries in test cases for CatalogD HA. Verified that
test cases failed without fix, and test cases were passed after fix.
- Passed core tests.
Change-Id: I5eada89052c5f16209c6f16357f30f78b4497434
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-server.h
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
M fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java
M tests/custom_cluster/test_catalogd_ha.py
7 files changed, 100 insertions(+), 84 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/58/20258/3
--
To view, visit http://gerrit.cloudera.org:8080/20258
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5eada89052c5f16209c6f16357f30f78b4497434
Gerrit-Change-Number: 20258
Gerrit-PatchSet: 3
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[Impala-ASF-CR] IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/20258 )
Change subject: IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20258/3/be/src/catalog/catalog-server.cc
File be/src/catalog/catalog-server.cc:
http://gerrit.cloudera.org:8080/#/c/20258/3/be/src/catalog/catalog-server.cc@410
PS3, Line 410: UpdateRegisteredCatalogd(active_catalogd_registration);
: if (FLAGS_force_catalogd_active && !IsActive()) {
> Ack
UpdateRegisteredCatalogd() is defined for RPC callback. We also define this RPC callback function for coordinator. Adding new parameter or changing return type don't make sense for coordinator. So I will not change this function and keep IsActive().
--
To view, visit http://gerrit.cloudera.org:8080/20258
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eada89052c5f16209c6f16357f30f78b4497434
Gerrit-Change-Number: 20258
Gerrit-PatchSet: 3
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 24 Jul 2023 22:23:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20258 )
Change subject: IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/13621/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20258
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eada89052c5f16209c6f16357f30f78b4497434
Gerrit-Change-Number: 20258
Gerrit-PatchSet: 1
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Sun, 23 Jul 2023 23:35:50 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/20258 )
Change subject: IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
......................................................................
IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
In IMPALA-12286, catalogd re-generate its Catalog Service ID in
JniCatalog when it becomes active. But CatalogServiceCatalog is not
updated when new Catalog Service ID is generated. This causes
coordinator hanging when processing DDLs.
In CatalogServer class, is_active_ is not protected by mutex
catalog_lock_, and pending_topic_updates_ is not cleared when the
catalogd becomes active. It's possible catalog server sends pending
catalog topic updates with old Catalog Service ID then sends catalog
topic updates with new Catalog Service ID.
This patch removes catalogServiceId_ from CatalogServiceCatalog, and
makes CatalogServiceCatalog to call JniCatalog.getServiceId() directly.
It makes is_active_ to be protected by mutex catalog_lock_, and makes
catalog server to clear pending_topic_updates_ when the catalogd becomes
active.
Testing:
- Added more queries in test cases for CatalogD HA. Verified that
test cases failed without fix, and test cases were passed after fix.
- Passed core tests.
Change-Id: I5eada89052c5f16209c6f16357f30f78b4497434
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-server.h
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
M fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java
M tests/custom_cluster/test_catalogd_ha.py
7 files changed, 100 insertions(+), 84 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/58/20258/2
--
To view, visit http://gerrit.cloudera.org:8080/20258
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5eada89052c5f16209c6f16357f30f78b4497434
Gerrit-Change-Number: 20258
Gerrit-PatchSet: 2
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
[Impala-ASF-CR] IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/20258 )
Change subject: IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
......................................................................
Patch Set 3:
(1 comment)
I'm sorry, I have one more question.
http://gerrit.cloudera.org:8080/#/c/20258/3/be/src/catalog/catalog-server.cc
File be/src/catalog/catalog-server.cc:
http://gerrit.cloudera.org:8080/#/c/20258/3/be/src/catalog/catalog-server.cc@410
PS3, Line 410: UpdateRegisteredCatalogd(active_catalogd_registration);
: if (FLAGS_force_catalogd_active && !IsActive()) {
I'm still not sure about this line. Can you explain what happen if two Catalogd start with FLAGS_force_catalogd_active == true? Is it possible for both to fail or both start as active?
What is different between UpdateRegisteredCatalogd as direct call in line 410 and ass statestore_subscriber_ callback in line 402? Is it possible to have two concurrent UpdateRegisteredCatalogd around this callsite?
Is it cleaner to change UpdateRegisteredCatalogd to return is_final_ instead of void? That way, this can be written as:
if (!UpdateRegisteredCatalogd(active_catalogd_registration) && FLAGS_force_catalogd_active) {
And then IsActive() can be removed.
--
To view, visit http://gerrit.cloudera.org:8080/20258
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eada89052c5f16209c6f16357f30f78b4497434
Gerrit-Change-Number: 20258
Gerrit-PatchSet: 3
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Mon, 24 Jul 2023 21:25:17 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/20258 )
Change subject: IMPALA-12305: Fix wrong Catalog Service ID when CatalogD becomes active
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/20258/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/20258/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@245
PS1, Line 245: private final Object catalogServiceIdLock_ = new Object();
I'm not sure if we do need a new lock here. 'catalogServiceId_' comes from JniCatalog which provides JniCatalog.getServiceId() to get it. There is already a lock in JniCatalog protecting the access. Maybe it's simpler to use JniCatalog.getServiceId() directly in getCatalogServiceId() and don't need the new get/set methods.
http://gerrit.cloudera.org:8080/#/c/20258/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3534
PS1, Line 3534: synchronized (catalogServiceIdLock_) {
This will synchronize all DDL/DMLs that need this id. I think read-write lock might be better since most of the cases are just reading this id.
http://gerrit.cloudera.org:8080/#/c/20258/1/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:
http://gerrit.cloudera.org:8080/#/c/20258/1/fe/src/main/java/org/apache/impala/service/JniCatalog.java@257
PS1, Line 257: synchronized (catalogServiceIdLock_) {
Also suggest chaning this to read-write lock
--
To view, visit http://gerrit.cloudera.org:8080/20258
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eada89052c5f16209c6f16357f30f78b4497434
Gerrit-Change-Number: 20258
Gerrit-PatchSet: 1
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Mon, 24 Jul 2023 03:01:30 +0000
Gerrit-HasComments: Yes