You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "Denverper (via GitHub)" <gi...@apache.org> on 2026/08/26 23:19:53 UTC
[PR] [Fix] Merge ContextAccessor service registrations in shaded bundle - Azure [iceberg]
Denverper opened a new pull request, #17841:
URL: https://github.com/apache/iceberg/pull/17841
The Azure bundle shades Reactor Core and Reactor Netty, which both contribute implementations to:
META-INF/services/io.micrometer.context.ContextAccessor
Because duplicate service descriptors are not merged by default, the resulting bundle preserves the Reactor Netty registration but drops reactor.util.context.ReactorContextAccessor. Although the implementation class remains in the JAR, Micrometer cannot discover it through ServiceLoader, resulting in runtime No ContextAccessor failures during Azure operations.
This change enables service-file merging for the Azure shadow JAR so both ContextAccessor implementations are preserved. It also adds an artifact-level test that verifies the completed bundle contains both registrations.
A backport to the Iceberg 1.10.x release line is requested because affected downstream releases are based on Iceberg 1.10.0.
--
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
Re: [PR] Azure: Merge ContextAccessor service registrations in shaded bundle [iceberg]
Posted by "Denverper (via GitHub)" <gi...@apache.org>.
Denverper commented on code in PR #17841:
URL: https://github.com/apache/iceberg/pull/17841#discussion_r3882908762
##########
azure-bundle/src/test/java/org/apache/iceberg/azure/TestAzureBundle.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.azure;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.jar.JarEntry;
+import java.util.jar.JarFile;
+import org.junit.jupiter.api.Test;
+
+// this test ensures the azure bundle service registrations contain all entries across all bundled deps
+// for ContextAccessor specifically, we expect entries from reactor core and reactor netty
+class TestAzureBundle {
+
+ @Test
+ void shadowJar_containsAllContextAccessorServiceRegistrations() throws IOException {
+ String bundlePath = System.getProperty("azure.test.bundle.jar");
+ assertThat(bundlePath).as("Azure bundle path").isNotNull();
+
+ try (JarFile bundle = new JarFile(new File(bundlePath))) {
+ JarEntry serviceDescriptor = bundle.getJarEntry("META-INF/services/io.micrometer.context.ContextAccessor");
+ assertThat(serviceDescriptor).as("ContextAccessor service descriptor").isNotNull();
+
+ try (InputStream input = bundle.getInputStream(serviceDescriptor)) {
+ assertThat(new String(input.readAllBytes(), UTF_8).split("\\R"))
+ .contains(
+ "reactor.netty.contextpropagation.ChannelContextAccessor",
+ "reactor.util.context.ReactorContextAccessor");
+ }
+ }
+ }
+}
Review Comment:
not sure of testing conventions here, was thinking pulling the jar and observing the entries should be sufficient. happy to change this in any way though!
also did a negative test by removing the merge to ensure failure
--
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
Re: [PR] Azure: Merge ContextAccessor service registrations in shaded bundle [iceberg]
Posted by "Denverper (via GitHub)" <gi...@apache.org>.
Denverper commented on code in PR #17841:
URL: https://github.com/apache/iceberg/pull/17841#discussion_r3882908762
##########
azure-bundle/src/test/java/org/apache/iceberg/azure/TestAzureBundle.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.azure;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.jar.JarEntry;
+import java.util.jar.JarFile;
+import org.junit.jupiter.api.Test;
+
+// this test ensures the azure bundle service registrations contain all entries across all bundled deps
+// for ContextAccessor specifically, we expect entries from reactor core and reactor netty
+class TestAzureBundle {
+
+ @Test
+ void shadowJar_containsAllContextAccessorServiceRegistrations() throws IOException {
+ String bundlePath = System.getProperty("azure.test.bundle.jar");
+ assertThat(bundlePath).as("Azure bundle path").isNotNull();
+
+ try (JarFile bundle = new JarFile(new File(bundlePath))) {
+ JarEntry serviceDescriptor = bundle.getJarEntry("META-INF/services/io.micrometer.context.ContextAccessor");
+ assertThat(serviceDescriptor).as("ContextAccessor service descriptor").isNotNull();
+
+ try (InputStream input = bundle.getInputStream(serviceDescriptor)) {
+ assertThat(new String(input.readAllBytes(), UTF_8).split("\\R"))
+ .contains(
+ "reactor.netty.contextpropagation.ChannelContextAccessor",
+ "reactor.util.context.ReactorContextAccessor");
+ }
+ }
+ }
+}
Review Comment:
not sure testing conventions here, was thinking pulling the jar and observing the entry should be sufficient. happy to change this in any way though!
##########
azure-bundle/src/test/java/org/apache/iceberg/azure/TestAzureBundle.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.azure;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.jar.JarEntry;
+import java.util.jar.JarFile;
+import org.junit.jupiter.api.Test;
+
+// this test ensures the azure bundle service registrations contain all entries across all bundled deps
+// for ContextAccessor specifically, we expect entries from reactor core and reactor netty
+class TestAzureBundle {
+
+ @Test
+ void shadowJar_containsAllContextAccessorServiceRegistrations() throws IOException {
+ String bundlePath = System.getProperty("azure.test.bundle.jar");
+ assertThat(bundlePath).as("Azure bundle path").isNotNull();
+
+ try (JarFile bundle = new JarFile(new File(bundlePath))) {
+ JarEntry serviceDescriptor = bundle.getJarEntry("META-INF/services/io.micrometer.context.ContextAccessor");
+ assertThat(serviceDescriptor).as("ContextAccessor service descriptor").isNotNull();
+
+ try (InputStream input = bundle.getInputStream(serviceDescriptor)) {
+ assertThat(new String(input.readAllBytes(), UTF_8).split("\\R"))
+ .contains(
+ "reactor.netty.contextpropagation.ChannelContextAccessor",
+ "reactor.util.context.ReactorContextAccessor");
+ }
+ }
+ }
+}
Review Comment:
not sure of testing conventions here, was thinking pulling the jar and observing the entry should be sufficient. happy to change this in any way though!
--
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
Re: [PR] Azure: Merge ContextAccessor service registrations in shaded bundle [iceberg]
Posted by "Denverper (via GitHub)" <gi...@apache.org>.
Denverper commented on code in PR #17841:
URL: https://github.com/apache/iceberg/pull/17841#discussion_r3867556462
##########
azure-bundle/build.gradle:
##########
@@ -42,6 +42,7 @@ project(":iceberg-azure-bundle") {
shadowJar {
archiveClassifier.set(null)
zip64 true
+ mergeServiceFiles()
Review Comment:
https://gradleup.com/shadow/configuration/merging/#merging-service-descriptor-files
--
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
Re: [PR] Azure: Merge ContextAccessor service registrations in shaded bundle [iceberg]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #17841:
URL: https://github.com/apache/iceberg/pull/17841#issuecomment-6306351212
Hi @Denverper thanks for finding this issue and creating a PR. While reviewing, I found that this is an issue for a lot of other bundles as well. I opened #18599 which contains a fix for all of them. I also included you as a co-author 😄
--
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
Re: [PR] Azure: Merge ContextAccessor service registrations in shaded bundle [iceberg]
Posted by "Denverper (via GitHub)" <gi...@apache.org>.
Denverper commented on PR #17841:
URL: https://github.com/apache/iceberg/pull/17841#issuecomment-5819432815
@singhpk234 friendly bump! 😄
--
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
Re: [PR] Azure: Merge ContextAccessor service registrations in shaded bundle [iceberg]
Posted by "Denverper (via GitHub)" <gi...@apache.org>.
Denverper commented on code in PR #17841:
URL: https://github.com/apache/iceberg/pull/17841#discussion_r3882908762
##########
azure-bundle/src/test/java/org/apache/iceberg/azure/TestAzureBundle.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.azure;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.jar.JarEntry;
+import java.util.jar.JarFile;
+import org.junit.jupiter.api.Test;
+
+// this test ensures the azure bundle service registrations contain all entries across all bundled deps
+// for ContextAccessor specifically, we expect entries from reactor core and reactor netty
+class TestAzureBundle {
+
+ @Test
+ void shadowJar_containsAllContextAccessorServiceRegistrations() throws IOException {
+ String bundlePath = System.getProperty("azure.test.bundle.jar");
+ assertThat(bundlePath).as("Azure bundle path").isNotNull();
+
+ try (JarFile bundle = new JarFile(new File(bundlePath))) {
+ JarEntry serviceDescriptor = bundle.getJarEntry("META-INF/services/io.micrometer.context.ContextAccessor");
+ assertThat(serviceDescriptor).as("ContextAccessor service descriptor").isNotNull();
+
+ try (InputStream input = bundle.getInputStream(serviceDescriptor)) {
+ assertThat(new String(input.readAllBytes(), UTF_8).split("\\R"))
+ .contains(
+ "reactor.netty.contextpropagation.ChannelContextAccessor",
+ "reactor.util.context.ReactorContextAccessor");
+ }
+ }
+ }
+}
Review Comment:
not sure of testing conventions here, thinking that pulling the jar and observing the entries should be sufficient. happy to change this in any way though!
also fysa did a negative test by removing the merge and saw a failure
--
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
Re: [PR] Azure: Merge ContextAccessor service registrations in shaded bundle [iceberg]
Posted by "Denverper (via GitHub)" <gi...@apache.org>.
Denverper commented on PR #17841:
URL: https://github.com/apache/iceberg/pull/17841#issuecomment-5622721648
@singhpk234 any NACKs to merging this now? 👀
(or if there are general waiting periods / windows to merge code here!)
--
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
Re: [PR] Azure: Merge ContextAccessor service registrations in shaded bundle [iceberg]
Posted by "Denverper (via GitHub)" <gi...@apache.org>.
Denverper commented on code in PR #17841:
URL: https://github.com/apache/iceberg/pull/17841#discussion_r3882908762
##########
azure-bundle/src/test/java/org/apache/iceberg/azure/TestAzureBundle.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.azure;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.jar.JarEntry;
+import java.util.jar.JarFile;
+import org.junit.jupiter.api.Test;
+
+// this test ensures the azure bundle service registrations contain all entries across all bundled deps
+// for ContextAccessor specifically, we expect entries from reactor core and reactor netty
+class TestAzureBundle {
+
+ @Test
+ void shadowJar_containsAllContextAccessorServiceRegistrations() throws IOException {
+ String bundlePath = System.getProperty("azure.test.bundle.jar");
+ assertThat(bundlePath).as("Azure bundle path").isNotNull();
+
+ try (JarFile bundle = new JarFile(new File(bundlePath))) {
+ JarEntry serviceDescriptor = bundle.getJarEntry("META-INF/services/io.micrometer.context.ContextAccessor");
+ assertThat(serviceDescriptor).as("ContextAccessor service descriptor").isNotNull();
+
+ try (InputStream input = bundle.getInputStream(serviceDescriptor)) {
+ assertThat(new String(input.readAllBytes(), UTF_8).split("\\R"))
+ .contains(
+ "reactor.netty.contextpropagation.ChannelContextAccessor",
+ "reactor.util.context.ReactorContextAccessor");
+ }
+ }
+ }
+}
Review Comment:
not sure of testing conventions here, was thinking pulling the jar and observing the entries should be sufficient. happy to change this in any way though!
also fysa did a negative test by removing the merge and saw a failure
--
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