falcon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "pavan kumar kolamuri" <pavan.kolam...@gmail.com>
Subject Re: Review Request 39588: State Store for instances scheduled by Falcon Native Scheduler
Date Thu, 12 Nov 2015 16:28:14 GMT


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/StateService.java, line 139
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114737#file1114737line139>
> >
> >     Why is it being overridden again after being fetched from statestore just 2
lines before.

Yes it is needed as awaiting predicates in instances updated in between and it needs to be
updated in DB


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/store/jdbc/BeanMapperUtil.java,
line 73
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114742#file1114742line73>
> >
> >     Can we use EntityState instead of instance, please?

Sure Thanks.


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/store/jdbc/BeanMapperUtil.java,
line 90
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114742#file1114742line90>
> >
> >     Let's not overload the word "instance" please. It is very confusing.

sure.


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/store/jdbc/BeanMapperUtil.java,
line 144
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114742#file1114742line144>
> >
> >     Didn't check the implementation but if an instance is created, shouldn't it
always be in some state?

Yes by default it should be in some state . Will remove this check. Even for type also


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/store/jdbc/BeanMapperUtil.java,
line 165
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114742#file1114742line165>
> >
> >     This will make database language dependent, as you will not be able to read
it in any other language except Java.

I understand . As there are complex data types involved, We feel this is best to use.


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/store/jdbc/BeanMapperUtil.java,
line 167
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114742#file1114742line167>
> >
> >     Any reason for not storing predicates in a simple readable format, like in their
own table/column? It would be immensely useful to read predicate states.

We don't want to use joins by storing perdicates in another table. We want to make it simple
thats why storing predicate as object itsself.


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/store/jdbc/BeanMapperUtil.java,
line 169
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114742#file1114742line169>
> >
> >     You should use IOUtils.closeQuietly() from commons.io.

Sure will use it.


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/store/jdbc/BeanMapperUtil.java,
line 172
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114742#file1114742line172>
> >
> >     It is not required to close it as it will be automatically closed when you call
close on out.

Sequence is changed . First we will close this before output stream


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/store/jdbc/BeanMapperUtil.java,
line 213
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114742#file1114742line213>
> >
> >     Use IOUtils.

sure


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/store/jdbc/BeanMapperUtil.java,
line 216
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114742#file1114742line216>
> >
> >     Redundant.

will remove


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/store/jdbc/BeanMapperUtil.java,
line 278
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114742#file1114742line278>
> >
> >     use IOUtils

ok


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/store/jdbc/BeanMapperUtil.java,
line 281
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114742#file1114742line281>
> >
> >     Redundant.

will remove


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/store/jdbc/EntityBean.java, line
39
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114743#file1114743line39>
> >
> >     nit: GET_ENTITIES_FOR_TYPE

Yes makes sense


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/store/jdbc/EntityBean.java, line
44
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114743#file1114743line44>
> >
> >     It's not a good convention to have table name in all caps or plural(makes it
difficult in certain scenarios to guess/spell correctly)

I think it is good to have Tables in Capitals, in queries also it is easy to differentiate
table names with column names and also OOZIE follows this convention


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/store/jdbc/EntityBean.java, line
65
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114743#file1114743line65>
> >
> >     Do you need the prefix current? There is no other similar column to be disambiguated.

Not sure why it is added like this. I followed from Base patch thinking in future some other
states will be added


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/store/jdbc/InstanceBean.java, line
41
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114744#file1114744line41>
> >
> >     Why like and not =? Can't this be done with an equal on entity_id?

Yes Ajay you are correct. EntityId was not there before , it was recently added, Thanks for
bringing this up.


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/store/jdbc/InstanceBean.java, line
43
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114744#file1114744line43>
> >
> >     Same as above for pattern matching on id.

Will take care


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/store/jdbc/InstanceBean.java, line
44
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114744#file1114744line44>
> >
> >     Same as above on pattern matching

Will take care


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/store/jdbc/InstanceBean.java, line
46
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114744#file1114744line46>
> >
> >     Why actualStartTime and actualEndTime? They are not deterministic so we always
query on the basis of nominal time.

Yes Completely agreed. We will discuss this with pallavi on this


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/store/jdbc/InstanceBean.java, line
47
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114744#file1114744line47>
> >
> >     There is no limit, so how is it ensuring that you only get the last instance?

Limit is not supported in open jpa as it is specific to few databases. But while retriving
we can specify query.setMaxResults(1) to make sure only one result will come.


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/store/jdbc/InstanceBean.java, line
48
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114744#file1114744line48>
> >
> >     DELETE_TABLE

Ok will make it DELETE_INSTANCES_TABLE. later if entities table also might need delete


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/store/jdbc/JDBCStateStore.java,
line 53
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114745#file1114745line53>
> >
> >     I understand that you might need clear method for tests but it is not useful,
rather risky, in production scenarios. So can we throw an exception here?

Sure will add debug statement over here


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/store/jdbc/JDBCStateStore.java,
line 151
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114745#file1114745line151>
> >
> >     Same as above for "clear"

sure will fix this.


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/store/service/FalconJPAService.java,
line 58
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114746#file1114746line58>
> >
> >     Can you please document the role of this field?

sure


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/store/service/FalconJPAService.java,
line 59
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114746#file1114746line59>
> >
> >     Make it final please.

sure will make


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/store/service/FalconJPAService.java,
line 82
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114746#file1114746line82>
> >
> >     .getSimpleName() will be more readable.

Sure will make


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java, line
52
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114747#file1114747line52>
> >
> >     What is the purpose of this field?

Will add comment . This represents whether DB Instance exists or not.


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java, line
53
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114747#file1114747line53>
> >
> >     Any reasons for this limitation? Most people will use MySQL or postgres in production
environments.

No reasons for limitation. I alpha phase we just want to test with Derby. It will work with
my sql also. We have to test it .


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java, line
129
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114747#file1114747line129>
> >
> >     redundant blank lines?

Just for more readability it was added. Will try to clean up


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java, line
137
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114747#file1114747line137>
> >
> >     "Falcon DB" is ambiguous.

Out of curious , Why Falcon DB is confusing and ambigous ? I don't understand


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java, line
151
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114747#file1114747line151>
> >
> >     Why double spacing everywhere?

Just for more readability as it is tool . Will remove it


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java, line
179
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114747#file1114747line179>
> >
> >     More descriptive message please.

I understand your concern. Done always comes with previous message with what is happening

Ex: 
Creating DB Table
DONE

If i add again Creating DB Table done it will redundant isn't it ?


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java, line
205
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114747#file1114747line205>
> >
> >     More descriptive message please.

Same i described above


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java, line
297
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114747#file1114747line297>
> >
> >     A more descriptive message on what operation got completed please. It will be
clearer in cases like calling one function from another.

Same as i mentioned earlier


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java, line
308
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114747#file1114747line308>
> >
> >     nit: Checking

Ok will fix


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java, line
329
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114747#file1114747line329>
> >
> >     More descriptive message please.

Described in previous comments


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java, line
360
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114747#file1114747line360>
> >
> >     nit: Validating

acked


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java, line
363
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114747#file1114747line363>
> >
> >     More descriptive message please.

Validating DB Connection.
DONE

I think this is enough for user to understand operation. Otherwise it looks redundant


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java, line
403
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114747#file1114747line403>
> >
> >     More descriptive message please.

Ok I will discuss with you offline


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/test/java/org/apache/falcon/state/service/store/TestJDBCStateStore.java,
line 127
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114756#file1114756line127>
> >
> >     Not the best way to do it. Break this into several test cases. Put expected
exception and message on each of them. You can extract common code to a private function.

Sure will fix this as part of another jira. As this is already big. Dont want to do bigger
items. Tracked in https://issues.apache.org/jira/browse/FALCON-1599


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/resources/falcon-buildinfo.properties, line 20
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114749#file1114749line20>
> >
> >     Is it required to have this copy in scheduler as well?

Yes it is required to get the version of falcon


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/resources/falcon-buildinfo.properties, line 1
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114749#file1114749line1>
> >
> >     Is another copy of this file required in scheduler module?

Yes required to get the falcon version during upgrade DB


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java, line
307
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114747#file1114747line307>
> >
> >     I think this function shouldn't throw exception on the basis of an argument.
Caller functions should decide how they want to handle the scenario and throw exception if
required.
> >     
> >     If you need to do it many times then create another function called assertFalconPropsTableExists()
which should throw an error.

Sure agreed will fix.


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java, line
307
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114747#file1114747line307>
> >
> >     Functions shouldn't throw exceptions on the basis of arguments. Let the caller
decide how he wants to handle it.
> >     
> >     If you require same exception throwing behavior then create another function
called assertFalconPropsTableExists which calls this function and throws error if it doesn't
exist.

Sure will fix


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java, line
301
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114747#file1114747line301>
> >
> >     There should only be a method called falconPropsTableExists() which returns
true or false.

Agreed will fix.


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java, line
301
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114747#file1114747line301>
> >
> >     There should be a function called falconPropsTableExists() and it should return
true or false depending on whether the table exists.

Sure will fix


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/store/jdbc/JDBCStateStore.java,
line 87
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114745#file1114745line87>
> >
> >     I guess empty is returned when there are no results, so when is null returned?

Will Fix


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > common/src/main/resources/startup.properties, line 261
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114729#file1114729line261>
> >
> >     I think we should store all these properties in a separate file to avoid bloating
of startup.properties. It will also help in avoiding the issue of credentials in startup.properties.

Yes we should do. Tracking in https://issues.apache.org/jira/browse/FALCON-1600


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/state/store/jdbc/JDBCStateStore.java,
line 42
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114745#file1114745line42>
> >
> >     This class should be divided into a class per table so that it becomes more
manageable.

Yes tracked in https://issues.apache.org/jira/browse/FALCON-1599


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java, line
44
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114747#file1114747line44>
> >
> >     How are we ensuring that this is not run by anyone else except admin?

Yes we should do tracked as part of https://issues.apache.org/jira/browse/FALCON-1601


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java, line
66
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114747#file1114747line66>
> >
> >     Can we rephrase it? It sounds like it is interactive. You should highlight that
it won't ask for confirmation.

Yes will do


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java, line
81
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114747#file1114747line81>
> >
> >     Falcon DB is very confusing. It should be either state store version or underlying
db version.

Will discuss offline with this


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java, line
141
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114747#file1114747line141>
> >
> >     We should document what will happen in case of rollbacks.

There is no roll back upgrade wont happen for the same version of Falcon


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java, line
186
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114747#file1114747line186>
> >
> >     Please avoid using Falcon DB for table names or database.

Will discuss offline regarding this


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java, line
216
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114747#file1114747line216>
> >
> >     Credentials shouldn't be stored in startup.properties

Yes we should fix. It is tracked in https://issues.apache.org/jira/browse/FALCON-1601


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/resources/META-INF/persistence.xml, line 1
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114748#file1114748line1>
> >
> >     It will be useful to at least put in comments the MySQL / Postgres configurations
wherever they are different.

will discuss this


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/test/resources/startup.properties, line 1
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114758#file1114758line1>
> >
> >     Having startup.properties in each module seems verbose to me. Can't we leverage
common startup.properties somehow?

Yes it is tracked in https://issues.apache.org/jira/browse/FALCON-1600


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > src/bin/falcon-db.sh, line 14
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114759#file1114759line14>
> >
> >     Sample usage/help documentation?

FalconStateStoreDBCLI Will print arguments if anything goes wrong.


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > src/bin/falcon-db.sh, line 16
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114759#file1114759line16>
> >
> >     Although not a hard requirement but it will be much better to have a python
script instead of a bash script. Reason for this is windows compatibility. We already have
a JIRA to convert existing scripts to python so we shouldn't add more tasks to it as much
as possible.

Tracked in https://issues.apache.org/jira/browse/FALCON-1600


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > src/conf/startup.properties, line 61
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114760#file1114760line61>
> >
> >     Shouldn't all scheduler code be inside a package path containing "scheduler"?

We will do as part of https://issues.apache.org/jira/browse/FALCON-1600


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java, line
403
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114747#file1114747line403>
> >
> >     nit: a more descriptive message on what action got completed will be more user
friendly.

As i said will discuss with this . I hope current one is enough


> On Nov. 5, 2015, 10:38 a.m., Ajay Yadava wrote:
> > scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java, line
363
> > <https://reviews.apache.org/r/39588/diff/2/?file=1114747#file1114747line363>
> >
> >     nit: DB Connection validated successfully.

acked


- pavan kumar


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39588/#review105205
-----------------------------------------------------------


On Nov. 3, 2015, 5:45 p.m., pavan kumar kolamuri wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39588/
> -----------------------------------------------------------
> 
> (Updated Nov. 3, 2015, 5:45 p.m.)
> 
> 
> Review request for Falcon.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/FALCON-1234
>     https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/FALCON-1234
> 
> 
> Repository: falcon-git
> 
> 
> Description
> -------
> 
> Persistent State Store for Falcon Native Scheduler
> 
> 
> Diffs
> -----
> 
>   checkstyle/src/main/resources/falcon/checkstyle.xml 2130e73 
>   checkstyle/src/main/resources/falcon/findbugs-exclude.xml 0a7580d 
>   common/src/main/resources/startup.properties cc5212a 
>   pom.xml 6f2c480 
>   scheduler/pom.xml 20a91d2 
>   scheduler/src/main/java/org/apache/falcon/execution/ExecutionInstance.java 3869ff2

>   scheduler/src/main/java/org/apache/falcon/execution/FalconExecutionService.java b959320

>   scheduler/src/main/java/org/apache/falcon/execution/ProcessExecutionInstance.java 19089c4

>   scheduler/src/main/java/org/apache/falcon/predicate/Predicate.java fb4ce82 
>   scheduler/src/main/java/org/apache/falcon/state/ID.java 420c856 
>   scheduler/src/main/java/org/apache/falcon/state/StateService.java 81357a4 
>   scheduler/src/main/java/org/apache/falcon/state/store/EntityStateStore.java 4aa6fdb

>   scheduler/src/main/java/org/apache/falcon/state/store/InMemoryStateStore.java 3822860

>   scheduler/src/main/java/org/apache/falcon/state/store/InstanceStateStore.java d6a4b49

>   scheduler/src/main/java/org/apache/falcon/state/store/StateStore.java f595c26 
>   scheduler/src/main/java/org/apache/falcon/state/store/jdbc/BeanMapperUtil.java PRE-CREATION

>   scheduler/src/main/java/org/apache/falcon/state/store/jdbc/EntityBean.java PRE-CREATION

>   scheduler/src/main/java/org/apache/falcon/state/store/jdbc/InstanceBean.java PRE-CREATION

>   scheduler/src/main/java/org/apache/falcon/state/store/jdbc/JDBCStateStore.java PRE-CREATION

>   scheduler/src/main/java/org/apache/falcon/state/store/service/FalconJPAService.java
PRE-CREATION 
>   scheduler/src/main/java/org/apache/falcon/tools/FalconStateStoreDBCLI.java PRE-CREATION

>   scheduler/src/main/resources/META-INF/persistence.xml PRE-CREATION 
>   scheduler/src/main/resources/falcon-buildinfo.properties PRE-CREATION 
>   scheduler/src/test/java/org/apache/falcon/execution/FalconExecutionServiceTest.java
b2f9e59 
>   scheduler/src/test/java/org/apache/falcon/notification/service/SchedulerServiceTest.java
b4a0f35 
>   scheduler/src/test/java/org/apache/falcon/state/AbstractSchedulerTestBase.java PRE-CREATION

>   scheduler/src/test/java/org/apache/falcon/state/EntityStateServiceTest.java 2f32b43

>   scheduler/src/test/java/org/apache/falcon/state/InstanceStateServiceTest.java d27ac7e

>   scheduler/src/test/java/org/apache/falcon/state/service/TestFalconJPAService.java PRE-CREATION

>   scheduler/src/test/java/org/apache/falcon/state/service/store/TestJDBCStateStore.java
PRE-CREATION 
>   scheduler/src/test/java/org/apache/falcon/tools/TestFalconStateStoreDBCLI.java PRE-CREATION

>   scheduler/src/test/resources/startup.properties PRE-CREATION 
>   src/bin/falcon-db.sh PRE-CREATION 
>   src/conf/startup.properties ce6e91f 
>   src/main/assemblies/distributed-package.xml 794eaef 
>   src/main/assemblies/standalone-package.xml fcff8d7 
>   unit/src/main/resources/startup.properties fe6f430 
> 
> Diff: https://reviews.apache.org/r/39588/diff/
> 
> 
> Testing
> -------
> 
> I have written unit tests. I will also test externally by setting up everything
> 
> 
> Thanks,
> 
> pavan kumar kolamuri
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message