Portability to Java 6 support #74

fredericBregier · 2019-06-18T11:16:53Z

This framework should be able to be binary compatible with Java 6 as
Netty does.
This merge request allows minor changes to be able to be compatible.

Also it upgrades dependencies.

All changes made by @marakiis

Issue reference #73

googlebot · 2019-06-18T11:16:56Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

fredericBregier · 2019-06-18T11:19:10Z

Signed the CLA
@marakiis Maybe you should too

googlebot · 2019-06-18T11:19:12Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

fredericBregier · 2019-06-18T11:55:49Z

I looked at the build from travis: it seems broken for long time, but on maven specification

<packaging>bundle</packaging>

So not related to this pull request.

anew

I started reviewing this... I think this introduces quite a lot of code that is a lot cleaner and more concise in Java 8. Given that Java 6 is long past its life time, I am not sure there is good enough justification for these changes. Maybe others have different opinions, but I personally would prefer keeping it at Java 8 language level.

anew · 2019-06-18T16:23:53Z

pom.xml

-    <rs-api.version>2.0</rs-api.version>
-    <netty.version>4.1.16.Final</netty.version>
-    <slf4j.version>1.7.5</slf4j.version>
+    <rs-api.version>2.1.1</rs-api.version>


is this related? Does Java6 support require changing the dependency versions?

Java6 does not required it, but it is the last supported version and Java6 still compatible.

anew · 2019-06-18T16:25:07Z

src/main/java/io/cdap/http/AbstractHttpResponder.java

@@ -53,7 +53,7 @@ public void sendString(HttpResponseStatus status, String data, HttpHeaders heade
      sendStatus(status, headers);
      return;
    }
-    ByteBuf buffer = Unpooled.wrappedBuffer(StandardCharsets.UTF_8.encode(data));
+    ByteBuf buffer = Unpooled.wrappedBuffer(Charset.forName("UTF-8").encode(data));


suggestion: create a static final UTF_8 and use it here. That comes closest to the current code.

anew · 2019-06-18T16:25:47Z

src/main/java/io/cdap/http/NettyHttpService.java

      }
      state = State.FAILED;
-      throw t;
+      throw (Exception) t;


This is not good practice. You do not know that this is an Exception, it could be any Error, and then the cast would fail.

Indeed! I didn't see this one (and some others). Done

anew · 2019-06-18T16:26:18Z

src/main/java/io/cdap/http/NettyHttpService.java

@@ -255,7 +254,7 @@ public synchronized void stop(long quietPeriod, long timeout, TimeUnit unit) thr
      }
    } catch (Throwable t) {
      state = State.FAILED;
-      throw t;
+      throw (Exception) t;


anew · 2019-06-18T16:27:07Z

src/main/java/io/cdap/http/SSLHandlerFactory.java

      KeyStore ks = KeyStore.getInstance("JKS");
      ks.load(is, keyStorePassword.toCharArray());
+      is.close();


this is not needed - the finally clause closes it anyway.

anew · 2019-06-18T16:28:03Z

src/main/java/io/cdap/http/internal/BasicHttpResponder.java

@@ -152,7 +151,8 @@ public void sendContent(HttpResponseStatus status, final BodyProducer bodyProduc
      // Response with error and close the connection
      sendContent(
        HttpResponseStatus.INTERNAL_SERVER_ERROR,
-        Unpooled.copiedBuffer("Failed to determined content length. Cause: " + t.getMessage(), StandardCharsets.UTF_8),
+        Unpooled.copiedBuffer("Failed to determined content length. Cause: " + t.getMessage(),


see my comment above on creating a constant

anew · 2019-06-18T16:28:15Z

src/main/java/io/cdap/http/internal/HandlerException.java

@@ -48,7 +48,7 @@

  HttpResponse createFailureResponse() {
    FullHttpResponse response = new DefaultFullHttpResponse(HttpVersion.HTTP_1_1, failureStatus,
-                                                            Unpooled.copiedBuffer(message, StandardCharsets.UTF_8));
+                                                            Unpooled.copiedBuffer(message, Charset.forName("UTF-8")));


anew · 2019-06-18T16:28:50Z

src/main/java/io/cdap/http/internal/HttpResourceHandler.java

@@ -149,6 +150,9 @@ public HttpResourceHandler(Iterable<? extends HttpHandler> handlers, Iterable<?
    if (method.isAnnotationPresent(DELETE.class)) {
      httpMethods.add(HttpMethod.DELETE);
    }
+    if (method.isAnnotationPresent(OPTIONS.class)) {


seems unrelated to Java6

Yes indeed, but we need to have OPTIONS support.

anew · 2019-06-18T16:30:14Z

src/main/java/io/cdap/http/internal/ImmutablePair.java

@@ -91,7 +91,7 @@ public String toString() {
   */
  @Override
  public int hashCode() {
-    return Objects.hash(first, second);
+    return Arrays.hashCode(new Object[]{first, second});


not the most efficient way - creating an array object just for computing a hash

In fact, that is the actual code behind Objects.hash (see Javadoc that explicitely defines it).

anew · 2019-06-18T16:31:52Z

src/main/java/io/cdap/http/internal/ImmutablePair.java

@@ -108,6 +108,33 @@ public boolean equals(Object o) {
      return false;
    }
    ImmutablePair<?, ?> other = (ImmutablePair<?, ?>) o;
-    return Objects.equals(first, other.first) && Objects.equals(second, other.second);
+
+    boolean fst;


consider a helper method that replaces Objects.equals().

fredericBregier · 2019-06-19T10:50:40Z

I correct this PR as your review.
I unterstand that keeping Java6 compatibility is a bit "old", but we (in our project) will move to Java7 and upward next year. In the meantime, we need Java6 compatibility. We hope those changes are very small and not being too ugly.

chtyim · 2019-06-19T15:56:04Z

It is better not to merge to develop branch, but rather a special branch for java 6 support only, so that we can release a special artifact for Java 6 only. In this way, it won’t affect the regular release of the Java 8 code base

fredericBregier · 2019-06-19T18:19:47Z

@chtyim Why not... As you wish. I will wait for your guidance.

The idea is to benefit of the project and to participate as much as we can too.

Maybe we could make after another PR with only the dependencies upgrade and the add of OPTIONS on develop?

…nce peek() implementation operates in a similar way to poll like a consumer which can lead to a multi-consumer scenario on a multi-producer-single-consumer queue, therefore getting into an infinite loop issue inside MPSC queue implementation.

Create 1.3.1 version

This framework should be able to be binary compatible with Java 6 as Netty does. This merge request allows minor changes to be able to be compatible. Also it upgrades dependencies and allows OPTION method (which was missing).

googlebot · 2019-10-30T14:58:40Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

fatso83 · 2022-04-19T15:11:50Z

Time to close this perhaps seeing it is moving nowhere? Too bad about the effort involved, but guessing Fred used this locally and has since upgraded the project to Java 7 (or 17?) 😄

anew reviewed Jun 18, 2019

View reviewed changes

fredericBregier force-pushed the java6 branch from be9bcf5 to 4d15bf9 Compare June 19, 2019 10:44

wyzhang and others added 4 commits September 19, 2019 08:37

Create 1.3.1 version

440c5c4

Merge pull request cdapio#78 from cdapio/wyzhang/release_1.3.1

e008f9a

Create 1.3.1 version

Portability to Java 6 support

cecead2

This framework should be able to be binary compatible with Java 6 as Netty does. This merge request allows minor changes to be able to be compatible. Also it upgrades dependencies and allows OPTION method (which was missing).

fredericBregier force-pushed the java6 branch from 4d15bf9 to cecead2 Compare October 30, 2019 14:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Portability to Java 6 support #74

Portability to Java 6 support #74

fredericBregier commented Jun 18, 2019

googlebot commented Jun 18, 2019

fredericBregier commented Jun 18, 2019

googlebot commented Jun 18, 2019

fredericBregier commented Jun 18, 2019

anew left a comment

anew Jun 18, 2019

fredericBregier Jun 19, 2019

anew Jun 18, 2019

fredericBregier Jun 19, 2019

anew Jun 18, 2019

fredericBregier Jun 19, 2019

anew Jun 18, 2019

fredericBregier Jun 19, 2019

anew Jun 18, 2019

fredericBregier Jun 19, 2019

anew Jun 18, 2019

fredericBregier Jun 19, 2019

anew Jun 18, 2019

fredericBregier Jun 19, 2019

anew Jun 18, 2019

fredericBregier Jun 19, 2019

anew Jun 18, 2019

fredericBregier Jun 19, 2019

anew Jun 18, 2019

fredericBregier Jun 19, 2019

fredericBregier commented Jun 19, 2019

chtyim commented Jun 19, 2019

fredericBregier commented Jun 19, 2019

googlebot commented Oct 30, 2019

fatso83 commented Apr 19, 2022

Portability to Java 6 support #74

Are you sure you want to change the base?

Portability to Java 6 support #74

Conversation

fredericBregier commented Jun 18, 2019

googlebot commented Jun 18, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

fredericBregier commented Jun 18, 2019

googlebot commented Jun 18, 2019

fredericBregier commented Jun 18, 2019

anew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredericBregier commented Jun 19, 2019

chtyim commented Jun 19, 2019

fredericBregier commented Jun 19, 2019

googlebot commented Oct 30, 2019

fatso83 commented Apr 19, 2022