[GitHub] flink pull request #5205: [FLINK-8037] Fix integer multiplication or shift i...

[greghogan]

Github user greghogan commented on a diff in the pull request:

https://github.com/apache/flink/pull/5205#discussion_r162199532
  
--- Diff: flink-core/src/main/java/org/apache/flink/util/AbstractID.java ---
@@ -186,7 +186,7 @@ private static long byteArrayToLong(byte[] ba, int 
offset) {
long l = 0;
 
for (int i = 0; i < SIZE_OF_LONG; ++i) {
-   l |= (ba[offset + SIZE_OF_LONG - 1 - i] & 0xffL) << (i 
<< 3);
+   l |= (ba[offset + SIZE_OF_LONG - 1 - i] & 0xffL) << 
((long) i << 3);
--- End diff --

Agreed, I don't see any reason why this would be flagged per the Java 
Language Specification. The nested shift also looks to be invalid according to 
the Java BNF per the Java Language Specification. Will revert this change.


---

[GitHub] flink pull request #5205: [FLINK-8037] Fix integer multiplication or shift i...

[pnowojski]

Github user pnowojski commented on a diff in the pull request:

https://github.com/apache/flink/pull/5205#discussion_r159904126
  
--- Diff: 
flink-connectors/flink-connector-kafka-0.11/src/main/java/org/apache/flink/streaming/connectors/kafka/FlinkKafkaProducer011.java
 ---
@@ -742,7 +742,7 @@ public void snapshotState(FunctionSnapshotContext 
context) throws Exception {
// case we adjust nextFreeTransactionalId by the range 
of transactionalIds that could be used for this
// scaling up.
if (getRuntimeContext().getNumberOfParallelSubtasks() > 
nextTransactionalIdHint.lastParallelism) {
-   nextFreeTransactionalId += 
getRuntimeContext().getNumberOfParallelSubtasks() * kafkaProducersPoolSize;
+   nextFreeTransactionalId += (long) 
getRuntimeContext().getNumberOfParallelSubtasks() * kafkaProducersPoolSize;
--- End diff --

Good change, although that is rather theoretical bug. To trigger it there 
would need to be more then 1_000_000 subtasks and more then 2000 parallel 
ongoing checkpoints. 


---

[GitHub] flink pull request #5205: [FLINK-8037] Fix integer multiplication or shift i...

[StephanEwen]

Github user StephanEwen commented on a diff in the pull request:

https://github.com/apache/flink/pull/5205#discussion_r159899525
  
--- Diff: flink-core/src/main/java/org/apache/flink/util/AbstractID.java ---
@@ -186,7 +186,7 @@ private static long byteArrayToLong(byte[] ba, int 
offset) {
long l = 0;
 
for (int i = 0; i < SIZE_OF_LONG; ++i) {
-   l |= (ba[offset + SIZE_OF_LONG - 1 - i] & 0xffL) << (i 
<< 3);
+   l |= (ba[offset + SIZE_OF_LONG - 1 - i] & 0xffL) << 
((long) i << 3);
--- End diff --

I think this might be not quite right - the number of bits to shift stays 
below 64 and should be correct as an int.


---

[GitHub] flink pull request #5205: [FLINK-8037] Fix integer multiplication or shift i...

[sunjincheng121]

Github user sunjincheng121 commented on a diff in the pull request:

https://github.com/apache/flink/pull/5205#discussion_r159222950
  
--- Diff: 
flink-runtime/src/test/java/org/apache/flink/runtime/operators/testutils/TaskCancelThread.java
 ---
@@ -49,7 +49,7 @@ public TaskCancelThread(int cancelTimeout, Thread 
interruptedThread, AbstractInv
@Override
public void run() {
try {
-   Thread.sleep(this.cancelTimeout*1000);
+   Thread.sleep(this.cancelTimeout*1000L);
--- End diff --

Add spaces between `*`


---

[GitHub] flink pull request #5205: [FLINK-8037] Fix integer multiplication or shift i...

[greghogan]

GitHub user greghogan opened a pull request:

https://github.com/apache/flink/pull/5205

[FLINK-8037] Fix integer multiplication or shift implicitly cast to long

## What is the purpose of the change

Fixes potential overflow flagged by the IntelliJ inspection "Integer 
multiplication or shift implicitly cast to long".

## Brief change log

- mark integer literal as long
- cast multiplied integer to long

## Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

## Does this pull request potentially affect one of the following parts:

  - Dependencies (does it add or upgrade a dependency): (yes / **no**)
  - The public API, i.e., is any changed class annotated with 
`@Public(Evolving)`: (yes / **no**)
  - The serializers: (yes / **no** / don't know)
  - The runtime per-record code paths (performance sensitive): (yes / 
**no** / don't know)
  - Anything that affects deployment or recovery: JobManager (and its 
components), Checkpointing, Yarn/Mesos, ZooKeeper: (yes / **no** / don't know)
  - The S3 file system connector: (yes / **no** / don't know)

## Documentation

  - Does this pull request introduce a new feature? (yes / **no)**
  - If yes, how is the feature documented? (**not applicable** / docs / 
JavaDocs / not documented)

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/greghogan/flink 
8037_fix_integer_multiplication_or_shift_implicitly_cast_to_long

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/flink/pull/5205.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #5205


commit ec2f80c5e890dc52b715eeca719c911aaa75fb82
Author: Greg Hogan 
Date:   2017-12-21T17:45:54Z

[FLINK-8037] Fix integer multiplication or shift implicitly cast to long




---