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

Already on GitHub? Sign in to your account

Minor changes to project #22

Merged
merged 27 commits into from Nov 30, 2012

Conversation

Projects
None yet
3 participants

Small changes to cleanup code and some minor errors

@dgomezferro dgomezferro and 1 other commented on an outdated diff Nov 28, 2012

.gitignore
@@ -0,0 +1,16 @@
+.classpath
+.project
+.settings
+.metadata
+target/
+bin/
@dgomezferro

dgomezferro Nov 28, 2012

Contributor

In bin/ we have startup script which I think makes sense to have versioned. We could discuss another structure for the project, but for the time being we don't want to .gitignore this.

@francisco-perez-sorrosal

francisco-perez-sorrosal Nov 28, 2012

Member

bin/ removed from file

@dgomezferro dgomezferro and 1 other commented on an outdated diff Nov 28, 2012

.gitignore
@@ -0,0 +1,16 @@
+.classpath
+.project
+.settings
+.metadata
+target/
+bin/
+lib/
+src/main/native/
@dgomezferro

dgomezferro Nov 28, 2012

Contributor

This way we are also ignoring the native code, I think we should move the compilation out of this directory instead.

@francisco-perez-sorrosal

francisco-perez-sorrosal Nov 28, 2012

Member

Filters added for not to take into account temporary files. Those filters will be removed when fixing the native code creation process.

@dgomezferro dgomezferro and 1 other commented on an outdated diff Nov 28, 2012

README.md
src/test/java/com/yahoo/omid/TestBasicTransaction.java
Logging
-------
-Logging can be adjusted in src/main/resource/log4j.properties.
-
+The logging preferences can be adjusted in src/main/resource/log4j.properties.
@dgomezferro

dgomezferro Nov 28, 2012

Contributor

There is a typo (it was there beforehand), it's "resources" (plural)

@dgomezferro dgomezferro and 1 other commented on an outdated diff Nov 28, 2012

pom.xml
- Unless required by applicable law or agreed to in writing, software
- distributed under the License is distributed on an "AS IS" BASIS,
- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- See the License for the specific language governing permissions and
- limitations under the License.
--->
+<!-- Licensed to the Apache Software Foundation (ASF) under one or more contributor
+ license agreements. See the NOTICE file distributed with this work for additional
+ information regarding copyright ownership. The ASF licenses this file to
+ You under the Apache License, Version 2.0 (the "License"); you may not use
+ this file except in compliance with the License. You may obtain a copy of
+ the License at http://www.apache.org/licenses/LICENSE-2.0 Unless required
+ by applicable law or agreed to in writing, software distributed under the
+ License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS
+ OF ANY KIND, either express or implied. See the License for the specific
+ language governing permissions and limitations under the License. -->
@dgomezferro

dgomezferro Nov 28, 2012

Contributor

We shouldn't reformat the license layout

@dgomezferro dgomezferro and 1 other commented on an outdated diff Nov 28, 2012

...main/java/com/yahoo/omid/client/TransactionState.java
this.startTimestamp = startTimestamp;;
this.commitTimestamp = 0;
- this.tsoclient = client;
@dgomezferro

dgomezferro Nov 28, 2012

Contributor

The tsoclient is used outside of this class. It should be private and made accessible through a getter maybe. With this change some tests fail due to a NPE

@dgomezferro dgomezferro and 1 other commented on an outdated diff Nov 28, 2012

src/test/java/com/yahoo/omid/OmidTestBase.java
@@ -48,17 +48,20 @@
public class OmidTestBase {
private static final Log LOG = LogFactory.getLog(OmidTestBase.class);
+ private static final ExecutorService bkExecutor = Executors.newSingleThreadExecutor();
+ private static final ExecutorService tsoExecutor = Executors.newSingleThreadExecutor();
@dgomezferro

dgomezferro Nov 28, 2012

Contributor

I like the usage of an ExecutorService, but this breaks running the tests from Eclipse. I think it works using maven because we fork per test (which we might need to revisit). The problem is submitting a new task after the executor has been shutdown. We can create them in setupOmid() instead.

@francisco-perez-sorrosal

francisco-perez-sorrosal Nov 28, 2012

Member

They've been defined as private "static ExecutorService xxx" and initialized in setupOmid() just as the HBase initialization is done.
I've also created ExecutorServices for bk and tso in TSOTestBase.java following the same approach as in this class.

@dgomezferro dgomezferro and 1 other commented on an outdated diff Nov 28, 2012

src/test/java/com/yahoo/omid/TestBasicTransaction.java
+ LOG.info("Last TS R1:" + lastTsRow1);
+ long previousTsRow1 = result1.raw()[1].getTimestamp();
+ LOG.info("Previous TS R1:" + previousTsRow1);
+
+ Get getResultRow2 = new Get(rowName2).setMaxVersions(2);
+ Result result2 = tt.get(getResultRow2);
+ byte[] val2 = result2.getValue(famName1, colName1);
+ assertTrue("Unexpected value for row 2 in col 1: " + Bytes.toString(val2),
+ Bytes.equals(dataValue4, result2.getValue(famName1, colName1)));
+ long lastTsRow2 = result2.raw()[0].getTimestamp();
+ LOG.info("Last TS R2:" + lastTsRow2);
+ long previousTsRow2 = result2.raw()[1].getTimestamp();
+ LOG.info("Previous TS R2:" + previousTsRow2);
+
+ LOG.info("***** ***** ***** ***** ***** ***** ***** ****** ***** ***** *****");
+ LOG.info("***** Why timestamps are not incremented sequentially??? *****");
@dgomezferro

dgomezferro Nov 28, 2012

Contributor

This should be removed, no? As to answering the question, both start timestamps and commit timestamps are assigned from the same "timestamp space"

@dgomezferro dgomezferro and 1 other commented on an outdated diff Nov 28, 2012

src/test/java/com/yahoo/omid/TestBasicTransaction.java
+ Bytes.equals(dataValue4, result2.getValue(famName1, colName1)));
+ long lastTsRow2 = result2.raw()[0].getTimestamp();
+ LOG.info("Last TS R2:" + lastTsRow2);
+ long previousTsRow2 = result2.raw()[1].getTimestamp();
+ LOG.info("Previous TS R2:" + previousTsRow2);
+
+ LOG.info("***** ***** ***** ***** ***** ***** ***** ****** ***** ***** *****");
+ LOG.info("***** Why timestamps are not incremented sequentially??? *****");
+ LOG.info("***** ***** ***** ***** ***** ***** ***** ****** ***** ***** *****");
+
+ assertTrue("Timestamps assigned by Tx2 to row 1 and row 2 are different", lastTsRow1 == lastTsRow2);
+ assertTrue("Timestamps assigned by Tx2 to row 1 and row 2 are different", previousTsRow1 == previousTsRow2);
+ assertTrue("Timestamp assigned by Tx2 to row 1 has not been increased monotonically", lastTsRow1 > previousTsRow1);
+ assertTrue("Timestamp assigned by Tx2 to row 2 has not been increased monotonically", lastTsRow2 > previousTsRow2);
+
+ } catch (TransactionException e) {
@dgomezferro

dgomezferro Nov 28, 2012

Contributor

I think it's better to throw the exceptions , otherwise if an unexpected exception happens we swallow it and the test passes, when it shouldn't

@francisco-perez-sorrosal

francisco-perez-sorrosal Nov 28, 2012

Member

Now exceptions are thrown directly in these two new tests. I'll do the same in other tests as soon as we define a format to follow for the whole project.

@dgomezferro dgomezferro and 1 other commented on an outdated diff Nov 28, 2012

src/test/java/com/yahoo/omid/TestUtils.java
+import java.io.IOException;
+import java.net.Socket;
+import java.net.UnknownHostException;
+
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+
+/**
+ *
+ * This class contains functionality that is useful for the Omid tests.
+ *
+ * @author Francisco Perez-Sorrosal - fperez@yahoo-inc.com
+ *
+ */
+public class TestUtils {
+ private static final Log LOG = LogFactory.getLog(TestUtils.class);
@dgomezferro

dgomezferro Nov 28, 2012

Contributor

Please use 4 spaces in this new file, sorry for not noticing before.

@francisco-perez-sorrosal

francisco-perez-sorrosal Nov 28, 2012

Member

Fixed. I didn't realized that I only modified eclipse to use spaces in text files and not in Java files. Now is configured properly.

@fpj fpj and 1 other commented on an outdated diff Nov 28, 2012

...main/java/com/yahoo/omid/client/TransactionState.java
@@ -19,6 +19,12 @@
import java.util.HashSet;
import java.util.Set;
+/**
+ * This class contains the required information to represent an Omid's transaction, including the set of rows modified.
+ *
+ * @author Francisco Perez-Sorrosal (fperez@yahoo-inc.com)
@fpj

fpj Nov 28, 2012

Contributor

In apache at least, we do not allow author tags.

Contributor

dgomezferro commented Nov 28, 2012

Thanks a lot for the cleanup!

I'm OK with merging, do you have more comments Flavio?

Contributor

fpj commented Nov 28, 2012

It looks great, thanks a lot for the changes, Francisco. It might be a good practice to add some description on top of new tests to state the purpose of the test. Although we can understand what it is doing by reading the code, it is difficult to make sure that future changes won't break it if we don't have a specification of what it is supposed to do.

Hi Flavio. Instead of adding a description to the tests, in the two new tests I've added, I've followed the approach of putting as the name of the test, the purpose of it. I think that for tests that are not very complex, this approach is OK. For complex tests that require more information, I agree that comments are the best practice. What do you think?

@francisco-perez-sorrosal francisco-perez-sorrosal pom and Makefile fixed to compile native code and place it in target/…
…. Makefile clean also integrated in pom to remove native library when doing mvn clean. pom now is formated with 4 spaces per tab
d7efbd6
Contributor

dgomezferro commented Nov 30, 2012

Please update the bin/omid.sh script to reflect the new placement of the native library.

Contributor

dgomezferro commented Nov 30, 2012

Great changes, thank you @francisco-perez-sorrosal !

Please merge

@francisco-perez-sorrosal francisco-perez-sorrosal merged commit 5f2fac9 into yahoo:master Nov 30, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment