-
Notifications
You must be signed in to change notification settings - Fork 18
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
#12. Use testcontainers. Use org.foundationdb artifacts and latest fdb image. #13
Conversation
Committer of one or more commits is not listed as a CLA signer, either individual or as a member of an organization. |
a50ba8b
to
b26f5fc
Compare
b26f5fc
to
f3a1478
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@farodin91 – could you please take a look at this PR? Looks like it might have some overlap with your PR #2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blindenvy thank you for the pr. I just found one issue and two small nits.
.gitignore
Outdated
jub*.xml | ||
|
||
# Eclipse Files # |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: It would better each user keep track of the editor specific files in their own global gitignore: https://stackoverflow.com/questions/7335420/global-git-ignore
pom.xml
Outdated
<artifactId>docker-compose-rule-junit4</artifactId> | ||
<version>0.33.0</version> | ||
<scope>test</scope> | ||
<groupId>org.testcontainers</groupId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: indent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpicks welcome, will fix.
@@ -41,18 +39,12 @@ public static ModifiableConfiguration getFoundationDBConfiguration(final String | |||
.set(STORAGE_BACKEND,"com.experoinc.janusgraph.diskstorage.foundationdb.FoundationDBStoreManager") | |||
.set(DIRECTORY, graphName) | |||
.set(DROP_ON_CLEAR, false) | |||
.set(CLUSTER_FILE_PATH, "src/test/resources/etc/fdb.cluster") | |||
.set(CLUSTER_FILE_PATH, "target/test-classes/fdb/fdb.cluster") | |||
.set(ISOLATION_LEVEL, "read_committed_with_write"); | |||
} | |||
|
|||
public static WriteConfiguration getFoundationDBGraphConfiguration() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would move all of these method into the foundationdb container as member. This would make it easier to replace the fixed port logic if foundationdb has a new client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure I can do that.
|
||
/** | ||
* @author Ted Wilmes ([email protected]) | ||
*/ | ||
@Ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you remove it my plan is to check all test when we start to add a CI solution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I will remove this.
I can reproduce your failing test case which also hangs for me. From what I see, the following line in Line 226 in f3a1478
What I don't know is if that is due to a logical error in the adapter or some sort of misconfiguration elsewhere. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you.
@blindenvy Could squash both commits?
@mbrukman My plan is to merge this first than work on travis ci which is afterward much easier.
@farodin91 wrote:
SGTM! |
…iondb artifacts and latest fdb image Signed-off-by: Christopher Jackson <[email protected]>
9573374
to
6550d34
Compare
@mbrukman Otherwise, i would merge it and start working on the travis ci integration. |
@farodin91 — looks like @blindenvy just squashed the commits and re-pushed, but FYI, GitHub has an option for "squash and merge" in the "Merge pull request" dropdown; I'm guessing you get to edit the combined squashed commit message. Feel free to use it in the future if you wish to speed up the process and let me know how it goes! |
@mbrukman I will let you know. Our normal review process requires two approvals or lazy consensus of 7 days. Therefore, i asked for an review. |
import java.time.Duration; | ||
import java.time.temporal.ChronoUnit; | ||
import static org.junit.Assert.*; | ||
import com.experoinc.janusgraph.FoundationDBContainer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am guessing this is separating the imports into (1) Java system, (2) third-party imports, (3) imports in this package. Is there a significant value in doing this? Especially after we rename com.experoinc.janusgraph
into org.janusgraph
, the third group will look very similar to the second group.
Can we just combine all the (non-static) imports into one section, and sort them all together?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is default behavior of eclipse when you run organize imports. I didn't do anything special or out of the ordinary here.
|
||
|
||
public FoundationDBContainer(String dockerImageName) { | ||
super(dockerImageName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're using 4-space indent in JanusGraph in general (including elsewhere in this repo as well), so we should follow the style here as well.
We can add an .editorconfig
later on to ensure that editors pick up this style in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected. I opened #22 to address general formatting/styling.
} catch (UnsupportedOperationException | IOException | InterruptedException e) { | ||
throw new ContainerLaunchException("Container startup failed. Failed to initialized the database."); | ||
} | ||
if(lsResult.getExitCode() != 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add space between if
and (
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected.
public void start() { | ||
super.start(); | ||
// initialize the database | ||
Container.ExecResult lsResult; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does lsResult
mean? Should it be called execResult
or configureResult
or something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed by using an appropriate variable name.
try (ServerSocket socket = new ServerSocket(0)) { | ||
return socket.getLocalPort(); | ||
} catch (Exception e) { | ||
System.err.println("Couldn't open random socket, using default!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe can also output what the default port value is, for folks who will be debugging this in the future?
Also, maybe we can update the message to:
Couldn't open random port; using default (%d).
It's confusing to see "couldn't open random socket, using default" since we don't have a default socket, just a default port.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improved wording as requested.
try { | ||
lsResult = this.execInContainer("fdbcli", "--exec", "configure new single ssd"); | ||
} catch (UnsupportedOperationException | IOException | InterruptedException e) { | ||
throw new ContainerLaunchException("Container startup failed. Failed to initialized the database."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception text here is capturing 3 exception types, so it's unclear WHY this error happened and what one needs to do differently to make it work.
It's also identical to the exception several lines down after the execution actually happens, but fails, and it also doesn't propagate the error message, which will make debugging harder in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improved error handling as requested. Improvements should make it easier for someone debugging to identify root cause.
Signed-off-by: Christopher Jackson <[email protected]>
@mbrukman please re-review when you can. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for making the changes!
@blindenvy — please feel free to squash and repush to make it easy to merge.
@farodin91 — please feel free to use GitHub's "squash and merge" if you're OK with the current state of the PR, given the latest updates.
If no objections I will leave it as is and we can use the 'squash and merge' functionality. |
All the test classes were passing except
FoundationDBPartitionGraphTest
. This test just seems to hang, it's unclear to me if this behavior is related to my change or is pre-existing or environmental. I have Ignored it for this PR. If I get feedback from others that this test is currently passing I will troubleshoot further, otherwise my intent is to leave it as Ignored and address it as a separate issue.