Well the reasoning for naming the snapshots was that the end user could provide a name for them globally across shards in the table. If we don't name them internally in each shard then we have to store the name and the mapping of the generation for each shard in the table. That actually sounds more complex, but it's doable. I have no preference at this point on which way to implement, however from the end user we should allow them to name the snapshots, or at the very least have a single identifier that they can reference for the entire snapshot (meaning one for the entire table to include all the shards).
Aaron On Thu, Oct 31, 2013 at 12:20 PM, rahul challapalli < [email protected]> wrote: > Its definitely easy to migrate to 4.4 and also maintain in the future if we > don't care about naming snapshots. But I think allowing the users to name a > snapshot adds more value. What do you think Aaron? > > Saurabh, > Did you get a chance to update the patch for > https://issues.apache.org/jira/browse/BLUR-208 to include merge policy > changes we discussed? > > - Rahul > > > On Wed, Oct 30, 2013 at 12:56 PM, saurabh gupta <[email protected] > >wrote: > > > Hi Rahul/Aaron, > > > > I was busy in some other work so I did not get time to look on this > story. > > But yesterday I think about the approach that we should take for > > SnapshotDeletionPolicy. If we want to keep the name of snapshot as user > > might need to give the name to them then we have to persist the mapping > of > > name of snapshot and corresponding generation somewhere in our system. My > > opinion is we should go with generation rather keeping name of snapshot. > > > > Please provide your feedback. > > > > Thanks, > > Saurabh Gupta > > > > > > > > On Sat, Oct 12, 2013 at 11:22 PM, saurabh gupta <[email protected] > > >wrote: > > > > > Hi Rahul, > > > > > > I have created sub tasks which are relevant to the story. Add more sub > > > tasks if you think the story should have. I have attached the patch > > within > > > the story. Review the changes and let me know if I can commit the > changes > > > into branch or not. > > > > > > Please provide the link for branch as well. > > > > > > Thanks, > > > Saurabh > > > > > > > > > On Sat, Oct 12, 2013 at 8:51 PM, rahul challapalli < > > > [email protected]> wrote: > > > > > >> Makes total sense Aaron. I was not sure whether to commit broken code > > into > > >> a repo. Thanks for your response. > > >> > > >> - Rahul > > >> On Oct 12, 2013 2:35 PM, "Aaron McCurry" <[email protected]> wrote: > > >> > > >> > For what it's worth, I would prefer that things that are not > compiling > > >> to > > >> > be left in an error state while we are working on a patch. > Especially > > >> in > > >> > branches like this where we are working together to resolve issues. > > The > > >> > biggest reason is because they can be easily identified and worked > on, > > >> if > > >> > they are commented out we would have to rely on @TODO or something > > else > > >> to > > >> > find the issues. > > >> > > > >> > So for the record, I'm perfectly ok with committing broken code into > > >> > feature branches. Just make a note in the commit that it's broken. > > >> > > > >> > Thanks! > > >> > > > >> > Aaron > > >> > > > >> > > > >> > On Sat, Oct 12, 2013 at 1:38 PM, rahul challapalli < > > >> > [email protected]> wrote: > > >> > > > >> > > Comment/Stub out the non-compiling code and provide the patch. > > Thanks. > > >> > > > > >> > > - Rahul > > >> > > On Oct 12, 2013 1:32 PM, "saurabh gupta" <[email protected]> > > >> wrote: > > >> > > > > >> > > > Hi Rahul, > > >> > > > > > >> > > > This is a good idea that you will create a branch. I will create > > sub > > >> > task > > >> > > > in story. Yes I have changed code. But BlurNRT has compilation > > error > > >> > due > > >> > > to > > >> > > > snapshotdeletionpolicy so suggest me whether I should create a > > >> patch or > > >> > > > not. > > >> > > > > > >> > > > Thanks, > > >> > > > Saurabh > > >> > > > > > >> > > > > > >> > > > On Fri, Oct 11, 2013 at 10:07 PM, rahul challapalli < > > >> > > > [email protected]> wrote: > > >> > > > > > >> > > > > Hi Sourabh, > > >> > > > > > > >> > > > > I will create a branch for this over the weekend. Can you > kindly > > >> > > create a > > >> > > > > sub-task to make snapshots work with lucene-4.4? > > >> > > > > We can work on this in small steps. If you have some work done > > on > > >> > this, > > >> > > > > provide a patch and someone will be able to review and apply > it. > > >> Let > > >> > me > > >> > > > > know what you think. > > >> > > > > > > >> > > > > You can use this link to create patches > > >> > > > > > > >> > > > > >> > http://ariejan.net/2009/10/26/how-to-create-and-apply-a-patch-with-git/ > > >> > > > > > > >> > > > > - Rahul > > >> > > > > > > >> > > > > > > >> > > > > On Thu, Oct 10, 2013 at 5:44 PM, Aaron McCurry < > > >> [email protected]> > > >> > > > wrote: > > >> > > > > > > >> > > > > > On Tue, Oct 8, 2013 at 11:35 PM, rahul challapalli < > > >> > > > > > [email protected]> wrote: > > >> > > > > > > > >> > > > > > > Thanks Saurabh for clarifying. > > >> > > > > > > > > >> > > > > > > Looks like we have to modify the existing code to make it > > work > > >> > with > > >> > > > > > Lucene > > >> > > > > > > 4.4 > > >> > > > > > > > > >> > > > > > > I see 2 approaches that we can take here : > > >> > > > > > > > > >> > > > > > > 1. We can write our own SanpshotDeletionPolicy if we > want > > >> users > > >> > > to > > >> > > > be > > >> > > > > > > able to give names to snapshots (I can imagine people > using > > >> date > > >> > as > > >> > > > > part > > >> > > > > > of > > >> > > > > > > the name) > > >> > > > > > > > > >> > > > > > > > >> > > > > > I would prefer this option, the writer is very heavy weight > as > > >> > Rahul > > >> > > > has > > >> > > > > > stated. > > >> > > > > > > > >> > > > > > > > >> > > > > > > 2. We can use PersistentSanpshotDeletionPolicy. The only > > >> reason > > >> > > we > > >> > > > > did > > >> > > > > > > not use it was because it uses IndexWriter for persisting > > >> which > > >> > is > > >> > > a > > >> > > > > very > > >> > > > > > > heavy object. > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > I guess we should create a separate branch for this and > also > > >> add > > >> > a > > >> > > > > > subtask > > >> > > > > > > for making snapshots work. Would like to hear some > thoughts > > >> from > > >> > > > others > > >> > > > > > as > > >> > > > > > > well. Thanks > > >> > > > > > > > > >> > > > > > > > >> > > > > > Sounds good. > > >> > > > > > > > >> > > > > > > > >> > > > > > > > > >> > > > > > > - Rahul > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > On Tue, Oct 8, 2013 at 7:23 PM, Aaron McCurry < > > >> > [email protected]> > > >> > > > > > wrote: > > >> > > > > > > > > >> > > > > > > > Hmm, I see what you saying let me take a closer look at > it > > >> and > > >> > > > report > > >> > > > > > > back. > > >> > > > > > > > > > >> > > > > > > > Aaron > > >> > > > > > > > > > >> > > > > > > > > > >> > > > > > > > On Tue, Oct 8, 2013 at 12:48 PM, saurabh gupta < > > >> > > > > [email protected] > > >> > > > > > > > >wrote: > > >> > > > > > > > > > >> > > > > > > > > Hi, > > >> > > > > > > > > > > >> > > > > > > > > In BlurNRT class there is a code which loads the > > existing > > >> > > > > snapshots: > > >> > > > > > > > > > > >> > > > > > > > > if (snapshotsDirectoryExists()) { > > >> > > > > > > > > // load existing snapshots > > >> > > > > > > > > sdp = new > > >> > > > > > > > > > > >> > SnapshotDeletionPolicy(_tableContext.getIndexDeletionPolicy(), > > >> > > > > > > > > loadExistingSnapshots()); > > >> > > > > > > > > } else { > > >> > > > > > > > > sdp = new > > >> > > > > > > > > > > >> > SnapshotDeletionPolicy(_tableContext.getIndexDeletionPolicy()); > > >> > > > > > > > > } > > >> > > > > > > > > > > >> > > > > > > > > But now in 4.4 version there is no constructor with > > second > > >> > > > > argument. > > >> > > > > > > They > > >> > > > > > > > > changed it corresponding to > > >> > > > > > > > > LUCENE-4973< > > >> http://issues.apache.org/jira/browse/LUCENE-4973 > > >> > > > > >> > > > > > > > > . > > >> > > > > > > > > > > >> > > > > > > > > Also to open a old snapshot the > > >> > > > > > > > > > > >> > > > > > > > > IndexCommit snapshot = snapshotter.getSnapshot(name); > > >> > > > > > > > > > > >> > > > > > > > > changed to > > >> > > > > > > > > > > >> > > > > > > > > IndexCommit snapshot = snapshotter.getIndexCommit(long > > >> gen); > > >> > > > > > > > > > > >> > > > > > > > > which takes generation > > >> > > > > > > > > > > >> > > > > > > > > I am not getting how to get the generation. > > >> > > > > > > > > > > >> > > > > > > > > I hope you understand what I am trying to say. > > >> > > > > > > > > > > >> > > > > > > > > Thanks, > > >> > > > > > > > > Saurabh Gupta > > >> > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > On Tue, Oct 8, 2013 at 2:54 AM, Aaron McCurry < > > >> > > > [email protected]> > > >> > > > > > > > wrote: > > >> > > > > > > > > > > >> > > > > > > > > > On Mon, Oct 7, 2013 at 4:54 PM, saurabh gupta < > > >> > > > > > [email protected] > > >> > > > > > > > > > >> > > > > > > > > > wrote: > > >> > > > > > > > > > > > >> > > > > > > > > > > Hi > > >> > > > > > > > > > > > > >> > > > > > > > > > > I am looking an improvement BLUR-208. I am stuck > at > > >> one > > >> > > place > > >> > > > > in > > >> > > > > > > > > BlurNRT > > >> > > > > > > > > > > class where we are loading previous snapshots and > > set > > >> in > > >> > > > > > > > > > > SnapshotDeletionPolicy. But now there is no way to > > >> load > > >> > > > > previous > > >> > > > > > > > > > snapshots. > > >> > > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > We haven't built a way to load in previous > snapshots. > > >> We > > >> > > have > > >> > > > > > > planned > > >> > > > > > > > on > > >> > > > > > > > > > doing so but have not actually implemented it yet. > We > > >> left > > >> > > the > > >> > > > > > > > snapshots > > >> > > > > > > > > > incomplete because at the time we were trying to > write > > >> an > > >> > > > > > InputFormat > > >> > > > > > > > for > > >> > > > > > > > > > Hadoop. > > >> > > > > > > > > > > > >> > > > > > > > > > It shouldn't be too hard to do if the table is > > offline. > > >> > > Online > > >> > > > > > moves > > >> > > > > > > > to > > >> > > > > > > > > > previous snapshots will be tricky. > > >> > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > Also SnapshotDeletionPolicy is returning list of > > >> > > IndexCommit > > >> > > > > and > > >> > > > > > > > > > otherwise > > >> > > > > > > > > > > we have to use generation to get the specific > > >> > IndexCommit. > > >> > > > Now > > >> > > > > I > > >> > > > > > > dont > > >> > > > > > > > > > know > > >> > > > > > > > > > > how to get the generation. > > >> > > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > Are you asking about how it works in Lucene or in > > Blur? > > >> In > > >> > > > Blur > > >> > > > > we > > >> > > > > > > > > create > > >> > > > > > > > > > a snapshot label to manage the snapshots. > > >> > > > > > > > > > > > >> > > > > > > > > > The Lucene basic code needed to open an old snapshot > > >> would > > >> > be > > >> > > > > > > something > > >> > > > > > > > > > like: > > >> > > > > > > > > > > > >> > > > > > > > > > IndexCommit snapshot = > > >> snapshotter.getSnapshot(name); > > >> > > > > > > > > > > > >> > > > > > > > > > DirectoryReader.open(snapshot); > > >> > > > > > > > > > If you could let us know what / how you would expect > > >> Blur > > >> > to > > >> > > > > behave > > >> > > > > > > > when > > >> > > > > > > > > > loading an old snapshot that would be great. Real > > world > > >> > use > > >> > > > > cases > > >> > > > > > > are > > >> > > > > > > > > the > > >> > > > > > > > > > best to work toward. > > >> > > > > > > > > > > > >> > > > > > > > > > Thanks! > > >> > > > > > > > > > > > >> > > > > > > > > > Aaron > > >> > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > Can anyone help me? > > >> > > > > > > > > > > > > >> > > > > > > > > > > Thanks > > >> > > > > > > > > > > Saurabh > > >> > > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > > > > > > >
