Skip to content

TINKERPOP-2761: fix version key race in Manifest#1716

Merged
spmallette merged 1 commit intoapache:masterfrom
lionelfleury:fix/version
Jun 24, 2022
Merged

TINKERPOP-2761: fix version key race in Manifest#1716
spmallette merged 1 commit intoapache:masterfrom
lionelfleury:fix/version

Conversation

@lionelfleury
Copy link
Copy Markdown
Contributor

@lionelfleury lionelfleury commented Jun 17, 2022 •
edited
Loading

@spmallette
Copy link
Copy Markdown
Contributor

spmallette commented Jun 20, 2022

thanks for contributing, but i'm not sure i understand the discussion that was referenced and thus not sure i understand why the manifest version needs a special tinkerpopVersion. could you please explain further what you mean by a "race on version"?

@lionelfleury
Copy link
Copy Markdown
Contributor Author

lionelfleury commented Jun 21, 2022 •
edited
Loading

Sure. If we have multiple jars on the classpath, Manifests.read("version") might take the first resolved key with that name. Which might be from another library.

In the mentioned thread, it seems to read the version key from hbase jar.

@lionelfleury lionelfleury mentioned this pull request Jun 21, 2022
Merged
9 tasks
@lionelfleury lionelfleury changed the title fix version key race in Manifest TINKERPOP-2761 fix version key race in Manifest Jun 21, 2022
@lionelfleury lionelfleury changed the title TINKERPOP-2761 fix version key race in Manifest TINKERPOP-2761: fix version key race in Manifest Jun 21, 2022
@spmallette
Copy link
Copy Markdown
Contributor

spmallette commented Jun 21, 2022

In the mentioned thread, it seems to read the version key from hbase jar

sorry - i read that thread but github didn't display the discussion in whole and i missed some additional comments

as i recall there isn't any standard to manifest files keys. rather than camel case of tinkerpopVersion could you please change it to tinkerpop-version ?

@lionelfleury
Copy link
Copy Markdown
Contributor Author

lionelfleury commented Jun 21, 2022

done ✅

@lionelfleury
TINKERPOP-2761: fix version key race in Manifest
bcb8408 
- use a more specific key `tinkerpop-version` to avoid race on `version`
- see: JanusGraph/janusgraph#3004

Signed-off-by: Lionel Fleury <lionel_fleury@hotmail.com>
li-boxuan
li-boxuan approved these changes Jun 23, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@li-boxuan li-boxuan left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@spmallette spmallette merged commit 9e41f43 into apache:master Jun 24, 2022
@lionelfleury lionelfleury deleted the fix/version branch July 6, 2022 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@li-boxuan li-boxuan li-boxuan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@lionelfleury @spmallette @li-boxuan