You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nastra (via GitHub)" <gi...@apache.org> on 2023/03/02 18:10:10 UTC
[GitHub] [iceberg] nastra opened a new pull request, #6990: Core: Use Awaitility instead of Thread.sleep() for async testing
nastra opened a new pull request, #6990:
URL: https://github.com/apache/iceberg/pull/6990
This uses https://github.com/awaitility/awaitility instead of `Thread.sleep()` to fix some flakiness in TestRESTCatalog.
fixes #6988
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #6990: Core: Use Awaitility instead of Thread.sleep() for async testing
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #6990:
URL: https://github.com/apache/iceberg/pull/6990#discussion_r1123532700
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -1037,71 +1037,75 @@ public void testCatalogTokenRefresh() throws Exception {
catalog.initialize(
"prod", ImmutableMap.of(CatalogProperties.URI, "ignored", "credential", "catalog:secret"));
- Thread.sleep(3_000); // sleep until after 2 refresh calls
-
- // call client credentials with no initial auth
- Mockito.verify(adapter)
- .execute(
- eq(HTTPMethod.POST),
- eq("v1/oauth/tokens"),
- any(),
- any(),
- eq(OAuthTokenResponse.class),
- eq(emptyHeaders),
- any());
-
- // use the client credential token for config
- Mockito.verify(adapter)
- .execute(
- eq(HTTPMethod.GET),
- eq("v1/config"),
- any(),
- any(),
- eq(ConfigResponse.class),
- eq(catalogHeaders),
- any());
-
- // verify the first token exchange
- Map<String, String> firstRefreshRequest =
- ImmutableMap.of(
- "grant_type", "urn:ietf:params:oauth:grant-type:token-exchange",
- "subject_token", "client-credentials-token:sub=catalog",
- "subject_token_type", "urn:ietf:params:oauth:token-type:access_token",
- "scope", "catalog");
- Mockito.verify(adapter)
- .execute(
- eq(HTTPMethod.POST),
- eq("v1/oauth/tokens"),
- any(),
- Mockito.argThat(firstRefreshRequest::equals),
- eq(OAuthTokenResponse.class),
- eq(catalogHeaders),
- any());
-
- // verify that a second exchange occurs
- Map<String, String> secondRefreshRequest =
- ImmutableMap.of(
- "grant_type", "urn:ietf:params:oauth:grant-type:token-exchange",
- "subject_token", "token-exchange-token:sub=client-credentials-token:sub=catalog",
- "subject_token_type", "urn:ietf:params:oauth:token-type:access_token",
- "scope", "catalog");
- Map<String, String> secondRefreshHeaders =
- ImmutableMap.of(
- "Authorization",
- "Bearer token-exchange-token:sub=client-credentials-token:sub=catalog");
- Mockito.verify(adapter)
- .execute(
- eq(HTTPMethod.POST),
- eq("v1/oauth/tokens"),
- any(),
- Mockito.argThat(secondRefreshRequest::equals),
- eq(OAuthTokenResponse.class),
- eq(secondRefreshHeaders),
- any());
+ Awaitility.await()
Review Comment:
We're relying here on the default polling interval (100 millis), which should be fine for these tests, but note that this can be modified as outlined in https://github.com/awaitility/awaitility/wiki/Usage#polling
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] danielcweeks merged pull request #6990: Core: Use Awaitility instead of Thread.sleep() for async testing
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks merged PR #6990:
URL: https://github.com/apache/iceberg/pull/6990
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #6990: Core: Use Awaitility instead of Thread.sleep() for async testing
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #6990:
URL: https://github.com/apache/iceberg/pull/6990#discussion_r1123532700
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -1037,71 +1037,75 @@ public void testCatalogTokenRefresh() throws Exception {
catalog.initialize(
"prod", ImmutableMap.of(CatalogProperties.URI, "ignored", "credential", "catalog:secret"));
- Thread.sleep(3_000); // sleep until after 2 refresh calls
-
- // call client credentials with no initial auth
- Mockito.verify(adapter)
- .execute(
- eq(HTTPMethod.POST),
- eq("v1/oauth/tokens"),
- any(),
- any(),
- eq(OAuthTokenResponse.class),
- eq(emptyHeaders),
- any());
-
- // use the client credential token for config
- Mockito.verify(adapter)
- .execute(
- eq(HTTPMethod.GET),
- eq("v1/config"),
- any(),
- any(),
- eq(ConfigResponse.class),
- eq(catalogHeaders),
- any());
-
- // verify the first token exchange
- Map<String, String> firstRefreshRequest =
- ImmutableMap.of(
- "grant_type", "urn:ietf:params:oauth:grant-type:token-exchange",
- "subject_token", "client-credentials-token:sub=catalog",
- "subject_token_type", "urn:ietf:params:oauth:token-type:access_token",
- "scope", "catalog");
- Mockito.verify(adapter)
- .execute(
- eq(HTTPMethod.POST),
- eq("v1/oauth/tokens"),
- any(),
- Mockito.argThat(firstRefreshRequest::equals),
- eq(OAuthTokenResponse.class),
- eq(catalogHeaders),
- any());
-
- // verify that a second exchange occurs
- Map<String, String> secondRefreshRequest =
- ImmutableMap.of(
- "grant_type", "urn:ietf:params:oauth:grant-type:token-exchange",
- "subject_token", "token-exchange-token:sub=client-credentials-token:sub=catalog",
- "subject_token_type", "urn:ietf:params:oauth:token-type:access_token",
- "scope", "catalog");
- Map<String, String> secondRefreshHeaders =
- ImmutableMap.of(
- "Authorization",
- "Bearer token-exchange-token:sub=client-credentials-token:sub=catalog");
- Mockito.verify(adapter)
- .execute(
- eq(HTTPMethod.POST),
- eq("v1/oauth/tokens"),
- any(),
- Mockito.argThat(secondRefreshRequest::equals),
- eq(OAuthTokenResponse.class),
- eq(secondRefreshHeaders),
- any());
+ Awaitility.await()
Review Comment:
We're relying here on the default polling delay, which should be fine for these tests, but note that this can be modified as outlined in https://github.com/awaitility/awaitility/wiki/Usage#polling
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #6990: Core: Use Awaitility instead of Thread.sleep() for async testing
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #6990:
URL: https://github.com/apache/iceberg/pull/6990#discussion_r1123532700
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -1037,71 +1037,75 @@ public void testCatalogTokenRefresh() throws Exception {
catalog.initialize(
"prod", ImmutableMap.of(CatalogProperties.URI, "ignored", "credential", "catalog:secret"));
- Thread.sleep(3_000); // sleep until after 2 refresh calls
-
- // call client credentials with no initial auth
- Mockito.verify(adapter)
- .execute(
- eq(HTTPMethod.POST),
- eq("v1/oauth/tokens"),
- any(),
- any(),
- eq(OAuthTokenResponse.class),
- eq(emptyHeaders),
- any());
-
- // use the client credential token for config
- Mockito.verify(adapter)
- .execute(
- eq(HTTPMethod.GET),
- eq("v1/config"),
- any(),
- any(),
- eq(ConfigResponse.class),
- eq(catalogHeaders),
- any());
-
- // verify the first token exchange
- Map<String, String> firstRefreshRequest =
- ImmutableMap.of(
- "grant_type", "urn:ietf:params:oauth:grant-type:token-exchange",
- "subject_token", "client-credentials-token:sub=catalog",
- "subject_token_type", "urn:ietf:params:oauth:token-type:access_token",
- "scope", "catalog");
- Mockito.verify(adapter)
- .execute(
- eq(HTTPMethod.POST),
- eq("v1/oauth/tokens"),
- any(),
- Mockito.argThat(firstRefreshRequest::equals),
- eq(OAuthTokenResponse.class),
- eq(catalogHeaders),
- any());
-
- // verify that a second exchange occurs
- Map<String, String> secondRefreshRequest =
- ImmutableMap.of(
- "grant_type", "urn:ietf:params:oauth:grant-type:token-exchange",
- "subject_token", "token-exchange-token:sub=client-credentials-token:sub=catalog",
- "subject_token_type", "urn:ietf:params:oauth:token-type:access_token",
- "scope", "catalog");
- Map<String, String> secondRefreshHeaders =
- ImmutableMap.of(
- "Authorization",
- "Bearer token-exchange-token:sub=client-credentials-token:sub=catalog");
- Mockito.verify(adapter)
- .execute(
- eq(HTTPMethod.POST),
- eq("v1/oauth/tokens"),
- any(),
- Mockito.argThat(secondRefreshRequest::equals),
- eq(OAuthTokenResponse.class),
- eq(secondRefreshHeaders),
- any());
+ Awaitility.await()
Review Comment:
the default poll interval is to poll every 200 millis with an initial delay of 100 millis. I think this is fine for these tests here but not that this can be modified as outlined in https://github.com/awaitility/awaitility/wiki/Usage#polling
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #6990: Core: Use Awaitility instead of Thread.sleep() for async testing
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #6990:
URL: https://github.com/apache/iceberg/pull/6990#discussion_r1123530262
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -1037,71 +1037,75 @@ public void testCatalogTokenRefresh() throws Exception {
catalog.initialize(
"prod", ImmutableMap.of(CatalogProperties.URI, "ignored", "credential", "catalog:secret"));
- Thread.sleep(3_000); // sleep until after 2 refresh calls
Review Comment:
there are no code differences in the test code itself, we're just wrapping it in a
```
Awaitility.await()
.atMost(5, TimeUnit.SECONDS)
.untilAsserted(
() -> { ... })
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org