You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2023/01/11 18:19:26 UTC
[GitHub] [beam] cushon opened a new pull request, #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
cushon opened a new pull request, #23210:
URL: https://github.com/apache/beam/pull/23210
in preparation for Java 17+. Follow-up to: https://github.com/apache/beam/commit/98f1f75459ee300baa5574042149a65063239705.
**Please** add a meaningful description for your change here
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [ ] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/get-started-contributing/#make-the-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Go+tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] cushon commented on a diff in pull request #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
Posted by "cushon (via GitHub)" <gi...@apache.org>.
cushon commented on code in PR #23210:
URL: https://github.com/apache/beam/pull/23210#discussion_r1097863820
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/coders/RowCoderGenerator.java:
##########
@@ -184,6 +186,30 @@ public static Coder<Row> generate(Schema schema) {
return rowCoder;
}
+ private static ClassLoadingStrategy<ClassLoader> getClassLoadingStrategy(Class<?> targetClass) {
+ try {
+ ClassLoadingStrategy<ClassLoader> strategy;
+ if (ClassInjector.UsingLookup.isAvailable()) {
+ Class<?> methodHandles = Class.forName("java.lang.invoke.MethodHandles");
+ Object lookup = methodHandles.getMethod("lookup").invoke(null);
+ Method privateLookupIn =
+ methodHandles.getMethod(
+ "privateLookupIn",
+ Class.class,
+ Class.forName("java.lang.invoke.MethodHandles$Lookup"));
+ Object privateLookup = privateLookupIn.invoke(null, targetClass, lookup);
+ strategy = ClassLoadingStrategy.UsingLookup.of(privateLookup);
+ } else if (ClassInjector.UsingReflection.isAvailable()) {
+ strategy = ClassLoadingStrategy.Default.INJECTION;
+ } else {
+ throw new IllegalStateException("No code generation strategy available");
+ }
+ return strategy;
+ } catch (ReflectiveOperationException e) {
+ throw new LinkageError(e.getMessage(), e);
+ }
+ }
+
Review Comment:
Done: I put the utility in `sdks/java/core/src/main/java/org/apache/beam/sdk/util/ByteBuddyUtils.java`, I'm happy to move it if you'd prefer somewhere else.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik merged pull request #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
Posted by "lukecwik (via GitHub)" <gi...@apache.org>.
lukecwik merged PR #23210:
URL: https://github.com/apache/beam/pull/23210
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] github-actions[bot] closed pull request #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
URL: https://github.com/apache/beam/pull/23210
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on a diff in pull request #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
Posted by "lukecwik (via GitHub)" <gi...@apache.org>.
lukecwik commented on code in PR #23210:
URL: https://github.com/apache/beam/pull/23210#discussion_r1103130852
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/util/ByteBuddyUtils.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.beam.sdk.util;
+
+import java.lang.reflect.Method;
+import net.bytebuddy.dynamic.loading.ClassInjector;
+import net.bytebuddy.dynamic.loading.ClassLoadingStrategy;
+
+/** Utilities for working with Byte Buddy. */
+@SuppressWarnings({
+ "nullness", // TODO(https://github.com/apache/beam/issues/20497)
Review Comment:
sgtm on narrowing the suppression to the single line
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] cushon commented on a diff in pull request #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
Posted by "cushon (via GitHub)" <gi...@apache.org>.
cushon commented on code in PR #23210:
URL: https://github.com/apache/beam/pull/23210#discussion_r1102218037
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/util/ByteBuddyUtils.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.beam.sdk.util;
+
+import java.lang.reflect.Method;
+import net.bytebuddy.dynamic.loading.ClassInjector;
+import net.bytebuddy.dynamic.loading.ClassLoadingStrategy;
+
+/** Utilities for working with Byte Buddy. */
+@SuppressWarnings({
+ "nullness", // TODO(https://github.com/apache/beam/issues/20497)
+})
+public final class ByteBuddyUtils {
+ private ByteBuddyUtils() {} // Non-instantiable
+
+ /** Returns a class loading strategy that is compatible with Java 17+. */
+ public static ClassLoadingStrategy<ClassLoader> getClassLoadingStrategy(Class<?> targetClass) {
+ try {
+ ClassLoadingStrategy<ClassLoader> strategy;
+ if (ClassInjector.UsingLookup.isAvailable()) {
+ Class<?> methodHandles = Class.forName("java.lang.invoke.MethodHandles");
Review Comment:
Will do. Do you want me to update the call sites to explicitly pass in classloaders, and if so which ones?
And to double-check: a classloader that loads `java.lang.*` classes itself instead of delegating to the system classloader is breaking the contract for custom classloaders. Do you definitely want to support that kind of non-delegating classloading 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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on a diff in pull request #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
Posted by "lukecwik (via GitHub)" <gi...@apache.org>.
lukecwik commented on code in PR #23210:
URL: https://github.com/apache/beam/pull/23210#discussion_r1103139757
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/util/ByteBuddyUtils.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.beam.sdk.util;
+
+import java.lang.reflect.Method;
+import net.bytebuddy.dynamic.loading.ClassInjector;
+import net.bytebuddy.dynamic.loading.ClassLoadingStrategy;
+
+/** Utilities for working with Byte Buddy. */
+@SuppressWarnings({
+ "nullness", // TODO(https://github.com/apache/beam/issues/20497)
+})
+public final class ByteBuddyUtils {
+ private ByteBuddyUtils() {} // Non-instantiable
+
+ /** Returns a class loading strategy that is compatible with Java 17+. */
+ public static ClassLoadingStrategy<ClassLoader> getClassLoadingStrategy(Class<?> targetClass) {
+ try {
+ ClassLoadingStrategy<ClassLoader> strategy;
+ if (ClassInjector.UsingLookup.isAvailable()) {
+ Class<?> methodHandles = Class.forName("java.lang.invoke.MethodHandles");
Review Comment:
Yes, there are some odd scenarios where users want to control the class loader like the threads context class loader.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] github-actions[bot] commented on pull request #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #23210:
URL: https://github.com/apache/beam/pull/23210#issuecomment-1245683736
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] cushon commented on a diff in pull request #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
Posted by "cushon (via GitHub)" <gi...@apache.org>.
cushon commented on code in PR #23210:
URL: https://github.com/apache/beam/pull/23210#discussion_r1102213268
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/util/ByteBuddyUtils.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.beam.sdk.util;
+
+import java.lang.reflect.Method;
+import net.bytebuddy.dynamic.loading.ClassInjector;
+import net.bytebuddy.dynamic.loading.ClassLoadingStrategy;
+
+/** Utilities for working with Byte Buddy. */
+@SuppressWarnings({
+ "nullness", // TODO(https://github.com/apache/beam/issues/20497)
Review Comment:
I added more targeted suppressions.
The checker framework makes conservative assumptions about reflection, I don't know a way to avoid suppressions here, e.g. when passing `null` to `Method.invoke`: https://checkerframework.org/manual/#reflection-resolution
Suggestions are welcome. Do you have a pointer to beam code that uses reflection and that doesn't have a class level `@SuppressWarnings("nullness")`, so I can follow the preferred style here? It looks like a lot of existing uses of reflection may have been added before the nullness analysis was turned on and have legacy suppressions.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] github-actions[bot] commented on pull request #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #23210:
URL: https://github.com/apache/beam/pull/23210#issuecomment-1313641508
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on pull request #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
Posted by "lukecwik (via GitHub)" <gi...@apache.org>.
lukecwik commented on PR #23210:
URL: https://github.com/apache/beam/pull/23210#issuecomment-1433949067
Run Java PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on pull request #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
Posted by "lukecwik (via GitHub)" <gi...@apache.org>.
lukecwik commented on PR #23210:
URL: https://github.com/apache/beam/pull/23210#issuecomment-1434899714
Run Java_Kafka_IO_Direct PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on pull request #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
Posted by "lukecwik (via GitHub)" <gi...@apache.org>.
lukecwik commented on PR #23210:
URL: https://github.com/apache/beam/pull/23210#issuecomment-1433851111
Run Java_Cdap_IO_Direct PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on pull request #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
Posted by "lukecwik (via GitHub)" <gi...@apache.org>.
lukecwik commented on PR #23210:
URL: https://github.com/apache/beam/pull/23210#issuecomment-1435110353
R: @bhisevishal
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] cushon commented on pull request #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
Posted by GitBox <gi...@apache.org>.
cushon commented on PR #23210:
URL: https://github.com/apache/beam/pull/23210#issuecomment-1245614002
R: @kileys
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on pull request #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
Posted by "lukecwik (via GitHub)" <gi...@apache.org>.
lukecwik commented on PR #23210:
URL: https://github.com/apache/beam/pull/23210#issuecomment-1433850998
Run Java PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on a diff in pull request #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
Posted by "lukecwik (via GitHub)" <gi...@apache.org>.
lukecwik commented on code in PR #23210:
URL: https://github.com/apache/beam/pull/23210#discussion_r1103143435
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/util/ByteBuddyUtils.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * 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.beam.sdk.util;
+
+import static java.util.Objects.requireNonNull;
+
+import java.lang.reflect.Method;
+import net.bytebuddy.dynamic.loading.ClassInjector;
+import net.bytebuddy.dynamic.loading.ClassLoadingStrategy;
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+/** Utilities for working with Byte Buddy. */
+public final class ByteBuddyUtils {
+ private ByteBuddyUtils() {} // Non-instantiable
+
+ /** Returns a class loading strategy that is compatible with Java 17+. */
+ public static ClassLoadingStrategy<ClassLoader> getClassLoadingStrategy(Class<?> targetClass) {
+ return getClassLoadingStrategy(targetClass, ByteBuddyUtils.class.getClassLoader());
+ }
+
+ /** Returns a class loading strategy that is compatible with Java 17+. */
+ public static ClassLoadingStrategy<ClassLoader> getClassLoadingStrategy(
+ Class<?> targetClass, @Nullable ClassLoader classLoader) {
+ try {
+ ClassLoadingStrategy<ClassLoader> strategy;
+ if (ClassInjector.UsingLookup.isAvailable()) {
Review Comment:
```suggestion
public static ClassLoadingStrategy<ClassLoader> getClassLoadingStrategy(Class<?> targetClass) {
try {
ClassLoadingStrategy<ClassLoader> strategy;
if (ClassInjector.UsingLookup.isAvailable()) {
ClassLoader classLoader = ReflectHelpers.getClassLoader(targetClass.getClassLoader());
```
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] cushon commented on a diff in pull request #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
Posted by "cushon (via GitHub)" <gi...@apache.org>.
cushon commented on code in PR #23210:
URL: https://github.com/apache/beam/pull/23210#discussion_r1102213268
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/util/ByteBuddyUtils.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.beam.sdk.util;
+
+import java.lang.reflect.Method;
+import net.bytebuddy.dynamic.loading.ClassInjector;
+import net.bytebuddy.dynamic.loading.ClassLoadingStrategy;
+
+/** Utilities for working with Byte Buddy. */
+@SuppressWarnings({
+ "nullness", // TODO(https://github.com/apache/beam/issues/20497)
Review Comment:
The checker framework makes conservative assumptions about reflection, I don't know a way to avoid suppressions here, e.g. when passing `null` to `Method.invoke`: https://checkerframework.org/manual/#reflection-resolution
Can you point me at an example of beam code that uses reflection and that doesn't have a class level `@SuppressWarnings("nullness")`, so I can follow the preferred style 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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] cushon commented on a diff in pull request #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
Posted by "cushon (via GitHub)" <gi...@apache.org>.
cushon commented on code in PR #23210:
URL: https://github.com/apache/beam/pull/23210#discussion_r1102193569
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/util/ByteBuddyUtils.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.beam.sdk.util;
+
+import java.lang.reflect.Method;
+import net.bytebuddy.dynamic.loading.ClassInjector;
+import net.bytebuddy.dynamic.loading.ClassLoadingStrategy;
+
+/** Utilities for working with Byte Buddy. */
+@SuppressWarnings({
+ "nullness", // TODO(https://github.com/apache/beam/issues/20497)
+})
+public final class ByteBuddyUtils {
+ private ByteBuddyUtils() {} // Non-instantiable
+
+ /** Returns a class loading strategy that is compatible with Java 17+. */
+ public static ClassLoadingStrategy<ClassLoader> getClassLoadingStrategy(Class<?> targetClass) {
+ try {
+ ClassLoadingStrategy<ClassLoader> strategy;
+ if (ClassInjector.UsingLookup.isAvailable()) {
+ Class<?> methodHandles = Class.forName("java.lang.invoke.MethodHandles");
+ Object lookup = methodHandles.getMethod("lookup").invoke(null);
+ Method privateLookupIn =
+ methodHandles.getMethod(
+ "privateLookupIn",
+ Class.class,
+ Class.forName("java.lang.invoke.MethodHandles$Lookup"));
+ Object privateLookup = privateLookupIn.invoke(null, targetClass, lookup);
+ strategy = ClassLoadingStrategy.UsingLookup.of(privateLookup);
+ } else if (ClassInjector.UsingReflection.isAvailable()) {
+ strategy = ClassLoadingStrategy.Default.INJECTION;
+ } else {
+ throw new IllegalStateException("No code generation strategy available");
+ }
+ return strategy;
+ } catch (ReflectiveOperationException e) {
+ throw new LinkageError(e.getMessage(), e);
Review Comment:
I think `LinkageError` is a fairly good unchecked analog to `ReflectiveOperationException`, but I'm happy to change it. What type of exception would you prefer?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on a diff in pull request #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
Posted by "lukecwik (via GitHub)" <gi...@apache.org>.
lukecwik commented on code in PR #23210:
URL: https://github.com/apache/beam/pull/23210#discussion_r1103133545
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/util/ByteBuddyUtils.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.beam.sdk.util;
+
+import java.lang.reflect.Method;
+import net.bytebuddy.dynamic.loading.ClassInjector;
+import net.bytebuddy.dynamic.loading.ClassLoadingStrategy;
+
+/** Utilities for working with Byte Buddy. */
+@SuppressWarnings({
+ "nullness", // TODO(https://github.com/apache/beam/issues/20497)
+})
+public final class ByteBuddyUtils {
+ private ByteBuddyUtils() {} // Non-instantiable
+
+ /** Returns a class loading strategy that is compatible with Java 17+. */
+ public static ClassLoadingStrategy<ClassLoader> getClassLoadingStrategy(Class<?> targetClass) {
+ try {
+ ClassLoadingStrategy<ClassLoader> strategy;
+ if (ClassInjector.UsingLookup.isAvailable()) {
+ Class<?> methodHandles = Class.forName("java.lang.invoke.MethodHandles");
+ Object lookup = methodHandles.getMethod("lookup").invoke(null);
+ Method privateLookupIn =
+ methodHandles.getMethod(
+ "privateLookupIn",
+ Class.class,
+ Class.forName("java.lang.invoke.MethodHandles$Lookup"));
+ Object privateLookup = privateLookupIn.invoke(null, targetClass, lookup);
+ strategy = ClassLoadingStrategy.UsingLookup.of(privateLookup);
+ } else if (ClassInjector.UsingReflection.isAvailable()) {
+ strategy = ClassLoadingStrategy.Default.INJECTION;
+ } else {
+ throw new IllegalStateException("No code generation strategy available");
+ }
+ return strategy;
+ } catch (ReflectiveOperationException e) {
+ throw new LinkageError(e.getMessage(), e);
Review Comment:
I would go with IllegalStateException
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] github-actions[bot] commented on pull request #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #23210:
URL: https://github.com/apache/beam/pull/23210#issuecomment-1336153449
This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any 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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on pull request #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
Posted by "lukecwik (via GitHub)" <gi...@apache.org>.
lukecwik commented on PR #23210:
URL: https://github.com/apache/beam/pull/23210#issuecomment-1433851269
Run Java_Hadoop_IO_Direct PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on a diff in pull request #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
Posted by "lukecwik (via GitHub)" <gi...@apache.org>.
lukecwik commented on code in PR #23210:
URL: https://github.com/apache/beam/pull/23210#discussion_r1102146563
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/util/ByteBuddyUtils.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.beam.sdk.util;
+
+import java.lang.reflect.Method;
+import net.bytebuddy.dynamic.loading.ClassInjector;
+import net.bytebuddy.dynamic.loading.ClassLoadingStrategy;
+
+/** Utilities for working with Byte Buddy. */
+@SuppressWarnings({
+ "nullness", // TODO(https://github.com/apache/beam/issues/20497)
+})
+public final class ByteBuddyUtils {
+ private ByteBuddyUtils() {} // Non-instantiable
+
+ /** Returns a class loading strategy that is compatible with Java 17+. */
+ public static ClassLoadingStrategy<ClassLoader> getClassLoadingStrategy(Class<?> targetClass) {
+ try {
+ ClassLoadingStrategy<ClassLoader> strategy;
+ if (ClassInjector.UsingLookup.isAvailable()) {
+ Class<?> methodHandles = Class.forName("java.lang.invoke.MethodHandles");
+ Object lookup = methodHandles.getMethod("lookup").invoke(null);
+ Method privateLookupIn =
+ methodHandles.getMethod(
+ "privateLookupIn",
+ Class.class,
+ Class.forName("java.lang.invoke.MethodHandles$Lookup"));
+ Object privateLookup = privateLookupIn.invoke(null, targetClass, lookup);
+ strategy = ClassLoadingStrategy.UsingLookup.of(privateLookup);
+ } else if (ClassInjector.UsingReflection.isAvailable()) {
+ strategy = ClassLoadingStrategy.Default.INJECTION;
+ } else {
+ throw new IllegalStateException("No code generation strategy available");
+ }
+ return strategy;
+ } catch (ReflectiveOperationException e) {
+ throw new LinkageError(e.getMessage(), e);
Review Comment:
I'm not sure we want to throw errors, it would make sense to throw a different type of runtime exception
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/util/ByteBuddyUtils.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.beam.sdk.util;
+
+import java.lang.reflect.Method;
+import net.bytebuddy.dynamic.loading.ClassInjector;
+import net.bytebuddy.dynamic.loading.ClassLoadingStrategy;
+
+/** Utilities for working with Byte Buddy. */
+@SuppressWarnings({
+ "nullness", // TODO(https://github.com/apache/beam/issues/20497)
Review Comment:
please don't add nullness suppression for new code
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/util/ByteBuddyUtils.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.beam.sdk.util;
+
+import java.lang.reflect.Method;
+import net.bytebuddy.dynamic.loading.ClassInjector;
+import net.bytebuddy.dynamic.loading.ClassLoadingStrategy;
+
+/** Utilities for working with Byte Buddy. */
+@SuppressWarnings({
+ "nullness", // TODO(https://github.com/apache/beam/issues/20497)
+})
+public final class ByteBuddyUtils {
+ private ByteBuddyUtils() {} // Non-instantiable
+
+ /** Returns a class loading strategy that is compatible with Java 17+. */
+ public static ClassLoadingStrategy<ClassLoader> getClassLoadingStrategy(Class<?> targetClass) {
+ try {
+ ClassLoadingStrategy<ClassLoader> strategy;
+ if (ClassInjector.UsingLookup.isAvailable()) {
+ Class<?> methodHandles = Class.forName("java.lang.invoke.MethodHandles");
Review Comment:
Please allow for the classloader to be passed in so that we can call `Class.forName("java.lang.invoke.MethodHandles", true, classLoader);`
This way the MethodHandles will be class loader sensitive for certain platforms like application servers.
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/coders/RowCoderGenerator.java:
##########
@@ -169,7 +169,7 @@ public static Coder<Row> generate(Schema schema) {
rowCoder =
builder
.make()
- .load(Coder.class.getClassLoader(), ClassLoadingStrategy.Default.INJECTION)
+ .load(Coder.class.getClassLoader(), getClassLoadingStrategy(Coder.class))
Review Comment:
```suggestion
.load(ReflectHelpers.findClassLoader(schema.getClass().getClassLoader()), getClassLoadingStrategy(schema.getClass()))
```
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/SelectByteBuddyHelpers.java:
##########
@@ -177,7 +177,7 @@ static RowSelector createRowSelector(SchemaAndDescriptor schemaAndDescriptor) {
return builder
.visit(new AsmVisitorWrapper.ForDeclaredMethods().writerFlags(ClassWriter.COMPUTE_FRAMES))
.make()
- .load(Row.class.getClassLoader(), ClassLoadingStrategy.Default.INJECTION)
+ .load(Row.class.getClassLoader(), getClassLoadingStrategy(Row.class))
Review Comment:
```suggestion
```
```suggestion
.load(ReflectHelpers.findClassLoader(schemaAndDescriptor.getClass().getClassLoader()), getClassLoadingStrategy(schemaAndDescriptor.getClass()))
```
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] cushon commented on a diff in pull request #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
Posted by "cushon (via GitHub)" <gi...@apache.org>.
cushon commented on code in PR #23210:
URL: https://github.com/apache/beam/pull/23210#discussion_r1102218037
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/util/ByteBuddyUtils.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.beam.sdk.util;
+
+import java.lang.reflect.Method;
+import net.bytebuddy.dynamic.loading.ClassInjector;
+import net.bytebuddy.dynamic.loading.ClassLoadingStrategy;
+
+/** Utilities for working with Byte Buddy. */
+@SuppressWarnings({
+ "nullness", // TODO(https://github.com/apache/beam/issues/20497)
+})
+public final class ByteBuddyUtils {
+ private ByteBuddyUtils() {} // Non-instantiable
+
+ /** Returns a class loading strategy that is compatible with Java 17+. */
+ public static ClassLoadingStrategy<ClassLoader> getClassLoadingStrategy(Class<?> targetClass) {
+ try {
+ ClassLoadingStrategy<ClassLoader> strategy;
+ if (ClassInjector.UsingLookup.isAvailable()) {
+ Class<?> methodHandles = Class.forName("java.lang.invoke.MethodHandles");
Review Comment:
I added an overload that takes a classloader.
Do you want me to update the call sites to explicitly pass in classloaders, and if so which ones?
And to double-check: a classloader that loads `java.lang.*` classes itself instead of delegating to the system classloader is breaking the contract for custom classloaders. Do you definitely want to support that kind of non-delegating classloading 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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] cushon commented on a diff in pull request #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
Posted by "cushon (via GitHub)" <gi...@apache.org>.
cushon commented on code in PR #23210:
URL: https://github.com/apache/beam/pull/23210#discussion_r1103176665
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/util/ByteBuddyUtils.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.beam.sdk.util;
+
+import java.lang.reflect.Method;
+import net.bytebuddy.dynamic.loading.ClassInjector;
+import net.bytebuddy.dynamic.loading.ClassLoadingStrategy;
+
+/** Utilities for working with Byte Buddy. */
+@SuppressWarnings({
+ "nullness", // TODO(https://github.com/apache/beam/issues/20497)
+})
+public final class ByteBuddyUtils {
+ private ByteBuddyUtils() {} // Non-instantiable
+
+ /** Returns a class loading strategy that is compatible with Java 17+. */
+ public static ClassLoadingStrategy<ClassLoader> getClassLoadingStrategy(Class<?> targetClass) {
+ try {
+ ClassLoadingStrategy<ClassLoader> strategy;
+ if (ClassInjector.UsingLookup.isAvailable()) {
+ Class<?> methodHandles = Class.forName("java.lang.invoke.MethodHandles");
+ Object lookup = methodHandles.getMethod("lookup").invoke(null);
+ Method privateLookupIn =
+ methodHandles.getMethod(
+ "privateLookupIn",
+ Class.class,
+ Class.forName("java.lang.invoke.MethodHandles$Lookup"));
+ Object privateLookup = privateLookupIn.invoke(null, targetClass, lookup);
+ strategy = ClassLoadingStrategy.UsingLookup.of(privateLookup);
+ } else if (ClassInjector.UsingReflection.isAvailable()) {
+ strategy = ClassLoadingStrategy.Default.INJECTION;
+ } else {
+ throw new IllegalStateException("No code generation strategy available");
+ }
+ return strategy;
+ } catch (ReflectiveOperationException e) {
+ throw new LinkageError(e.getMessage(), e);
Review Comment:
Done, thanks
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on pull request #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
Posted by "lukecwik (via GitHub)" <gi...@apache.org>.
lukecwik commented on PR #23210:
URL: https://github.com/apache/beam/pull/23210#issuecomment-1433850900
Run Java_Kafka_IO_Direct PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] kileys commented on a diff in pull request #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
Posted by GitBox <gi...@apache.org>.
kileys commented on code in PR #23210:
URL: https://github.com/apache/beam/pull/23210#discussion_r1067317189
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/coders/RowCoderGenerator.java:
##########
@@ -184,6 +186,30 @@ public static Coder<Row> generate(Schema schema) {
return rowCoder;
}
+ private static ClassLoadingStrategy<ClassLoader> getClassLoadingStrategy(Class<?> targetClass) {
+ try {
+ ClassLoadingStrategy<ClassLoader> strategy;
+ if (ClassInjector.UsingLookup.isAvailable()) {
+ Class<?> methodHandles = Class.forName("java.lang.invoke.MethodHandles");
+ Object lookup = methodHandles.getMethod("lookup").invoke(null);
+ Method privateLookupIn =
+ methodHandles.getMethod(
+ "privateLookupIn",
+ Class.class,
+ Class.forName("java.lang.invoke.MethodHandles$Lookup"));
+ Object privateLookup = privateLookupIn.invoke(null, targetClass, lookup);
+ strategy = ClassLoadingStrategy.UsingLookup.of(privateLookup);
+ } else if (ClassInjector.UsingReflection.isAvailable()) {
+ strategy = ClassLoadingStrategy.Default.INJECTION;
+ } else {
+ throw new IllegalStateException("No code generation strategy available");
+ }
+ return strategy;
+ } catch (ReflectiveOperationException e) {
+ throw new LinkageError(e.getMessage(), e);
+ }
+ }
+
Review Comment:
Can we do this once in a core.util file?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] cushon commented on a diff in pull request #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
Posted by "cushon (via GitHub)" <gi...@apache.org>.
cushon commented on code in PR #23210:
URL: https://github.com/apache/beam/pull/23210#discussion_r1113465324
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/coders/RowCoderGenerator.java:
##########
@@ -169,7 +169,7 @@ public static Coder<Row> generate(Schema schema) {
rowCoder =
builder
.make()
- .load(Coder.class.getClassLoader(), ClassLoadingStrategy.Default.INJECTION)
+ .load(Coder.class.getClassLoader(), getClassLoadingStrategy(Coder.class))
Review Comment:
The change here from `Coder.class.getClassLoader()` to `ReflectHelpers.findClassLoader(schema.getClass().getClassLoader())` is causing some of the failures from https://github.com/apache/beam/pull/25566
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] kileys commented on pull request #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
Posted by "kileys (via GitHub)" <gi...@apache.org>.
kileys commented on PR #23210:
URL: https://github.com/apache/beam/pull/23210#issuecomment-1409150903
@lukecwik
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] cushon commented on a diff in pull request #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
Posted by "cushon (via GitHub)" <gi...@apache.org>.
cushon commented on code in PR #23210:
URL: https://github.com/apache/beam/pull/23210#discussion_r1090974426
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/coders/RowCoderGenerator.java:
##########
@@ -184,6 +186,30 @@ public static Coder<Row> generate(Schema schema) {
return rowCoder;
}
+ private static ClassLoadingStrategy<ClassLoader> getClassLoadingStrategy(Class<?> targetClass) {
+ try {
+ ClassLoadingStrategy<ClassLoader> strategy;
+ if (ClassInjector.UsingLookup.isAvailable()) {
+ Class<?> methodHandles = Class.forName("java.lang.invoke.MethodHandles");
+ Object lookup = methodHandles.getMethod("lookup").invoke(null);
+ Method privateLookupIn =
+ methodHandles.getMethod(
+ "privateLookupIn",
+ Class.class,
+ Class.forName("java.lang.invoke.MethodHandles$Lookup"));
+ Object privateLookup = privateLookupIn.invoke(null, targetClass, lookup);
+ strategy = ClassLoadingStrategy.UsingLookup.of(privateLookup);
+ } else if (ClassInjector.UsingReflection.isAvailable()) {
+ strategy = ClassLoadingStrategy.Default.INJECTION;
+ } else {
+ throw new IllegalStateException("No code generation strategy available");
+ }
+ return strategy;
+ } catch (ReflectiveOperationException e) {
+ throw new LinkageError(e.getMessage(), e);
+ }
+ }
+
Review Comment:
That sounds good. I'm happy to consolidate it wherever you prefer, can you recommend a location? I wasn't sure what the best place for that shared utility would be.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on pull request #23210: Replace more uses of `ClassLoadingStrategy.Default.INJECTION`
Posted by "lukecwik (via GitHub)" <gi...@apache.org>.
lukecwik commented on PR #23210:
URL: https://github.com/apache/beam/pull/23210#issuecomment-1433949115
Run Java_Kafka_IO_Direct PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org