Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: encodeTuple int32 panic #11339

Closed
wants to merge 1 commit into from
Closed

fix: encodeTuple int32 panic #11339

wants to merge 1 commit into from

Conversation

linehk
Copy link
Contributor

@linehk linehk commented Apr 24, 2024

When using int32 type in tuple and Pack, error will occur:

panic: unencodable element at index 0 (1, type int32)

The following example can be reproduced:

package main

import (
	"github.com/apple/foundationdb/bindings/go/src/fdb"
	"github.com/apple/foundationdb/bindings/go/src/fdb/tuple"
)

func main() {
	fdb.MustAPIVersion(730)
	var i int32 = 1
	tuple.Tuple{i}.Pack()
}

This is because encodeTuple does not handle int32.

In the Go language, int and int32 are not the same type.

Fixed by adding support for int32.

Code-Reviewer Section

The general pull request guidelines can be found here.

Please check each of the following things and check all boxes before accepting a PR.

  • The PR has a description, explaining both the problem and the solution.
  • The description mentions which forms of testing were done and the testing seems reasonable.
  • Every function/class/actor that was touched is reasonably well documented.

For Release-Branches

If this PR is made against a release-branch, please also check the following:

  • This change/bugfix is a cherry-pick from the next younger branch (younger release-branch or main if this is the youngest branch)
  • There is a good reason why this PR needs to go into a release branch and this reason is documented (either in the description above or in a linked GitHub issue)

@jzhou77 jzhou77 closed this Apr 26, 2024
@jzhou77 jzhou77 reopened this Apr 26, 2024
@jzhou77 jzhou77 requested a review from johscheuer April 26, 2024 18:14
@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-clang-ide on Linux CentOS 7

  • Commit ID: 1b0209d
  • Duration 0:25:11
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

  • Commit ID: 1b0209d
  • Duration 0:34:01
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr on Linux CentOS 7

  • Commit ID: 1b0209d
  • Duration 0:35:26
  • Result: ❌ FAILED
  • Error: Error while executing command: python3 -m joshua.joshua start --tarball $(find build_output/packages -name correctness\*.tar.gz) --username ${CORRECTNESS_USERNAME} --max-runs 10000. Reason: exit status 2
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

  • Commit ID: 1b0209d
  • Duration 0:35:43
  • Result: ❌ FAILED
  • Error: Error while executing command: mvn install:install-file --batch-mode -Dorg.slf4j.simpleLogger.log.org.apache.maven.cli.transfer.Slf4jMavenTransferListener=warn -Dfile=packages/fdb-java-${FDB_VERSION}-SNAPSHOT.jar -DgroupId=org.foundationdb -DartifactId=fdb-java -Dversion=${FDB_VERSION}-SNAPSHOT -Dpackaging=jar -DgeneratePom=true. Reason: exit status 1
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)
  • Cluster Test Logs zip file of the test logs (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-macos on macOS Ventura 13.x

  • Commit ID: 1b0209d
  • Duration 0:48:28
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-clang on Linux CentOS 7

  • Commit ID: 1b0209d
  • Duration 0:54:19
  • Result: ❌ FAILED
  • Error: Error while executing command: if python3 -m joshua.joshua list --stopped | grep ${ENSEMBLE_ID} | grep -q 'pass=10[0-9][0-9][0-9]'; then echo PASS; else echo FAIL && exit 1; fi. Reason: exit status 1
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@linehk
Copy link
Contributor Author

linehk commented May 23, 2024

CI failed, is there anything I need to add?

@jzhou77 @johscheuer PTAL.

Copy link
Contributor

@johscheuer johscheuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, I somehow missed the review request.

First thanks for taking the time to raise this PR!

Based on my understanding it's expected that you will see an error when trying to encode an int32 as int32 is not supported by the Tuple Layer: https://github.com/apple/foundationdb/blob/main/bindings/go/src/fdb/tuple/tuple.go#L57-L59 and looking at other tuple implementations, e.g. the Java one: https://github.com/apple/foundationdb/blob/main/bindings/java/src/main/com/apple/foundationdb/tuple/Tuple.java, they only support long and BigInteger, which would translate to int64 and BitInt in go

It should be safe to convert an int32 to an int64 but the Tuple layer doesn't support this officially, so it would be better to handle that in your application code. Otherwise we should add int32 and uint32 to all other Tuple implementations too.

@linehk
Copy link
Contributor Author

linehk commented May 23, 2024

OK, thanks for the explanation. I will close this PR.

@linehk linehk closed this May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants