You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2020/04/16 13:11:35 UTC
[GitHub] [lucene-solr] s1monw opened a new pull request #1434: LUCENE-9324:
Add an ID to SegmentCommitInfo
s1monw opened a new pull request #1434: LUCENE-9324: Add an ID to SegmentCommitInfo
URL: https://github.com/apache/lucene-solr/pull/1434
We already have IDs in SegmentInfo, which are useful to uniquely identify segments. Having IDs on SegmentCommitInfo is be useful too in order to compare commits for equality and make snapshots incremental on generational files.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] s1monw commented on a change in pull request #1434:
LUCENE-9324: Add an ID to SegmentCommitInfo
Posted by GitBox <gi...@apache.org>.
s1monw commented on a change in pull request #1434: LUCENE-9324: Add an ID to SegmentCommitInfo
URL: https://github.com/apache/lucene-solr/pull/1434#discussion_r410382265
##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentCommitInfo.java
##########
@@ -388,4 +399,17 @@ public SegmentCommitInfo clone() {
final int getDelCount(boolean includeSoftDeletes) {
return includeSoftDeletes ? getDelCount() + getSoftDelCount() : getDelCount();
}
+
+ private void generationAdvanced() {
+ sizeInBytes = -1;
+ id = null;
+ }
+
+ public byte[] getId() {
+ if (id == null) {
+ // we advanced a generation - need to generate a new ID
+ id = StringHelper.randomId();
Review comment:
I changed this to roll over to a new ID each time the generation is incremented. I don't really like that we call `StringHelper.randomId()` over and over again. I wonder if it would be better to set a new ID from the outside just before we clone in order to commit instead of doing it inside SegmentCommitInfo. This would make things more explicit IMO
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] mikemccand commented on a change in pull request
#1434: LUCENE-9324: Add an ID to SegmentCommitInfo
Posted by GitBox <gi...@apache.org>.
mikemccand commented on a change in pull request #1434: LUCENE-9324: Add an ID to SegmentCommitInfo
URL: https://github.com/apache/lucene-solr/pull/1434#discussion_r410273671
##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentCommitInfo.java
##########
@@ -388,4 +399,17 @@ public SegmentCommitInfo clone() {
final int getDelCount(boolean includeSoftDeletes) {
return includeSoftDeletes ? getDelCount() + getSoftDelCount() : getDelCount();
}
+
+ private void generationAdvanced() {
+ sizeInBytes = -1;
+ id = null;
+ }
+
+ public byte[] getId() {
+ if (id == null) {
+ // we advanced a generation - need to generate a new ID
+ id = StringHelper.randomId();
Review comment:
I agree it's messy. It's just that this sounds like a simple getter w/o side effect, yet, it does have an important and surprising side effect of setting the `id`. It would be bad if two threads called it at once and got two different `id`s back.
If we really believe all callers are already sync'd (by IW's monitor lock), can we 1) add a comment stating this assumption, and 2) fix the assignment to do something like this instead:
```
byte[] newID = StringHelper.randomId();
assert id == null;
id = newID;
```
?
That is still best effort, but might catch us in tests if we have cases where two threads really are invoking `getId` at once.
Or, maybe we can change this to assign the `id` at the same time as the SCI was changed (e.g. `delGen` increased), instead of at `get` time?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] mikemccand commented on a change in pull request
#1434: LUCENE-9324: Add an ID to SegmentCommitInfo
Posted by GitBox <gi...@apache.org>.
mikemccand commented on a change in pull request #1434: LUCENE-9324: Add an ID to SegmentCommitInfo
URL: https://github.com/apache/lucene-solr/pull/1434#discussion_r409554301
##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentCommitInfo.java
##########
@@ -388,4 +399,17 @@ public SegmentCommitInfo clone() {
final int getDelCount(boolean includeSoftDeletes) {
return includeSoftDeletes ? getDelCount() + getSoftDelCount() : getDelCount();
}
+
+ private void generationAdvanced() {
+ sizeInBytes = -1;
+ id = null;
+ }
+
+ public byte[] getId() {
+ if (id == null) {
+ // we advanced a generation - need to generate a new ID
+ id = StringHelper.randomId();
Review comment:
Do we need some thread safety here?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] mikemccand commented on a change in pull request
#1434: LUCENE-9324: Add an ID to SegmentCommitInfo
Posted by GitBox <gi...@apache.org>.
mikemccand commented on a change in pull request #1434: LUCENE-9324: Add an ID to SegmentCommitInfo
URL: https://github.com/apache/lucene-solr/pull/1434#discussion_r410273911
##########
File path: lucene/core/src/java/org/apache/lucene/index/IndexWriter.java
##########
@@ -3081,7 +3081,7 @@ private SegmentCommitInfo copySegmentAsIs(SegmentCommitInfo info, String segName
info.info.getUseCompoundFile(), info.info.getCodec(),
info.info.getDiagnostics(), info.info.getId(), info.info.getAttributes(), info.info.getIndexSort());
SegmentCommitInfo newInfoPerCommit = new SegmentCommitInfo(newInfo, info.getDelCount(), info.getSoftDelCount(), info.getDelGen(),
- info.getFieldInfosGen(), info.getDocValuesGen());
+ info.getFieldInfosGen(), info.getDocValuesGen(), info.getId());
Review comment:
ok
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] mikemccand commented on a change in pull request
#1434: LUCENE-9324: Add an ID to SegmentCommitInfo
Posted by GitBox <gi...@apache.org>.
mikemccand commented on a change in pull request #1434: LUCENE-9324: Add an ID to SegmentCommitInfo
URL: https://github.com/apache/lucene-solr/pull/1434#discussion_r410266570
##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentInfos.java
##########
@@ -374,7 +376,15 @@ public static final SegmentInfos readCommit(Directory directory, ChecksumIndexIn
if (softDelCount + delCount > info.maxDoc()) {
throw new CorruptIndexException("invalid deletion count: " + softDelCount + delCount + " vs maxDoc=" + info.maxDoc(), input);
}
- SegmentCommitInfo siPerCommit = new SegmentCommitInfo(info, delCount, softDelCount, delGen, fieldInfosGen, dvGen);
+ final byte[] sciId;
+ if (format > VERSION_74) {
+ sciId = new byte[StringHelper.ID_LENGTH];
+ input.readBytes(sciId, 0, sciId.length);
+ } else {
+ sciId = infos.id;
+ // NOCOMMIT can we do this? it would at least give us consistent BWC but we can't identify the same SCI in different commits
Review comment:
+1 for `null` and have caller deal with the BWC. Stealing the `id` from another place (either "parent" `SegmentInfos` or the corresponding `SegmentInfo`) seems too dangerous.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] mikemccand commented on a change in pull request
#1434: LUCENE-9324: Add an ID to SegmentCommitInfo
Posted by GitBox <gi...@apache.org>.
mikemccand commented on a change in pull request #1434: LUCENE-9324: Add an ID to SegmentCommitInfo
URL: https://github.com/apache/lucene-solr/pull/1434#discussion_r409556321
##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentInfos.java
##########
@@ -374,7 +376,15 @@ public static final SegmentInfos readCommit(Directory directory, ChecksumIndexIn
if (softDelCount + delCount > info.maxDoc()) {
throw new CorruptIndexException("invalid deletion count: " + softDelCount + delCount + " vs maxDoc=" + info.maxDoc(), input);
}
- SegmentCommitInfo siPerCommit = new SegmentCommitInfo(info, delCount, softDelCount, delGen, fieldInfosGen, dvGen);
+ final byte[] sciId;
+ if (format > VERSION_74) {
+ sciId = new byte[StringHelper.ID_LENGTH];
+ input.readBytes(sciId, 0, sciId.length);
+ } else {
+ sciId = infos.id;
+ // NOCOMMIT can we do this? it would at least give us consistent BWC but we can't identify the same SCI in different commits
Review comment:
Good question ... maybe we could use `info.getId()`? Then it'd be unique across `SCI`, but, shared across `SI` which is also weird.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] s1monw merged pull request #1434: LUCENE-9324: Add an
ID to SegmentCommitInfo
Posted by GitBox <gi...@apache.org>.
s1monw merged pull request #1434: LUCENE-9324: Add an ID to SegmentCommitInfo
URL: https://github.com/apache/lucene-solr/pull/1434
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jpountz commented on a change in pull request #1434:
LUCENE-9324: Add an ID to SegmentCommitInfo
Posted by GitBox <gi...@apache.org>.
jpountz commented on a change in pull request #1434: LUCENE-9324: Add an ID to SegmentCommitInfo
URL: https://github.com/apache/lucene-solr/pull/1434#discussion_r410185034
##########
File path: lucene/backward-codecs/src/test/org/apache/lucene/index/TestBackwardsCompatibility.java
##########
@@ -862,7 +862,8 @@ public void testAddOldIndexesReader() throws IOException {
TestUtil.addIndexesSlowly(w, reader);
w.close();
reader.close();
-
+ SegmentInfos si = SegmentInfos.readLatestCommit(targetDir);
+ assertEquals("all SCIs should have an id now", 0, si.asList().stream().filter(sci -> sci.getId() == null).count());
Review comment:
here too?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jpountz commented on a change in pull request #1434:
LUCENE-9324: Add an ID to SegmentCommitInfo
Posted by GitBox <gi...@apache.org>.
jpountz commented on a change in pull request #1434: LUCENE-9324: Add an ID to SegmentCommitInfo
URL: https://github.com/apache/lucene-solr/pull/1434#discussion_r409866462
##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentInfos.java
##########
@@ -374,7 +376,15 @@ public static final SegmentInfos readCommit(Directory directory, ChecksumIndexIn
if (softDelCount + delCount > info.maxDoc()) {
throw new CorruptIndexException("invalid deletion count: " + softDelCount + delCount + " vs maxDoc=" + info.maxDoc(), input);
}
- SegmentCommitInfo siPerCommit = new SegmentCommitInfo(info, delCount, softDelCount, delGen, fieldInfosGen, dvGen);
+ final byte[] sciId;
+ if (format > VERSION_74) {
+ sciId = new byte[StringHelper.ID_LENGTH];
+ input.readBytes(sciId, 0, sciId.length);
+ } else {
+ sciId = infos.id;
+ // NOCOMMIT can we do this? it would at least give us consistent BWC but we can't identify the same SCI in different commits
Review comment:
Ah sorry I got confused because I thought that "parent" was referring to SegmentInfo (no s) instead of SegmentInfos, but I agree that SegmentInfos is not great either.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] s1monw commented on a change in pull request #1434:
LUCENE-9324: Add an ID to SegmentCommitInfo
Posted by GitBox <gi...@apache.org>.
s1monw commented on a change in pull request #1434: LUCENE-9324: Add an ID to SegmentCommitInfo
URL: https://github.com/apache/lucene-solr/pull/1434#discussion_r409851521
##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentInfos.java
##########
@@ -374,7 +376,15 @@ public static final SegmentInfos readCommit(Directory directory, ChecksumIndexIn
if (softDelCount + delCount > info.maxDoc()) {
throw new CorruptIndexException("invalid deletion count: " + softDelCount + delCount + " vs maxDoc=" + info.maxDoc(), input);
}
- SegmentCommitInfo siPerCommit = new SegmentCommitInfo(info, delCount, softDelCount, delGen, fieldInfosGen, dvGen);
+ final byte[] sciId;
+ if (format > VERSION_74) {
+ sciId = new byte[StringHelper.ID_LENGTH];
+ input.readBytes(sciId, 0, sciId.length);
+ } else {
+ sciId = infos.id;
+ // NOCOMMIT can we do this? it would at least give us consistent BWC but we can't identify the same SCI in different commits
Review comment:
> I don't understand the idea of stealing the parent ID, wouldn't it cause Lucene to consider commits equal when they are not, which would be a much worse problem than considering commits different when they are equal?
the idea was to use the `SegmentInfos` ID which is different for every commit as a default. It would not cause Lucene to consider commits equal when they are not. I just moved to using null instead, it was an idea that has downsides too we can just go with null. The real question is when do we assign an ID then? Once we write the SCI again even if it didn't change? I think we should but that would then bring back the same problem with the fallback.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jpountz commented on a change in pull request #1434:
LUCENE-9324: Add an ID to SegmentCommitInfo
Posted by GitBox <gi...@apache.org>.
jpountz commented on a change in pull request #1434: LUCENE-9324: Add an ID to SegmentCommitInfo
URL: https://github.com/apache/lucene-solr/pull/1434#discussion_r410189327
##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentInfos.java
##########
@@ -374,7 +376,18 @@ public static final SegmentInfos readCommit(Directory directory, ChecksumIndexIn
if (softDelCount + delCount > info.maxDoc()) {
throw new CorruptIndexException("invalid deletion count: " + softDelCount + delCount + " vs maxDoc=" + info.maxDoc(), input);
}
- SegmentCommitInfo siPerCommit = new SegmentCommitInfo(info, delCount, softDelCount, delGen, fieldInfosGen, dvGen);
+ final byte[] sciId;
+ if (format > VERSION_74 ) {
+ if (input.readByte() == 1) {
Review comment:
can you be stricter and throw a CorruptIndexException if the byte is neither 0 nor 1?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jpountz commented on a change in pull request #1434:
LUCENE-9324: Add an ID to SegmentCommitInfo
Posted by GitBox <gi...@apache.org>.
jpountz commented on a change in pull request #1434: LUCENE-9324: Add an ID to SegmentCommitInfo
URL: https://github.com/apache/lucene-solr/pull/1434#discussion_r410186856
##########
File path: lucene/backward-codecs/src/test/org/apache/lucene/index/TestBackwardsCompatibility.java
##########
@@ -1367,6 +1368,16 @@ public void testIndexCreatedVersion() throws IOException {
}
}
+ public void testSegmentCommitInfoId() throws IOException {
+ for (String name : oldNames) {
+ Directory dir = oldIndexDirs.get(name);
+ SegmentInfos infos = SegmentInfos.readLatestCommit(dir);
+ for (SegmentCommitInfo info : infos) {
+ assertNull(info.getId());
Review comment:
we need to make this assert depend on the version to be future-proof? Ie.
```
if (info.info.getVersion().onOrAfter(Version.LUCENE_8_6_0)) {
assertNotNull(info.toString(), info.getId());
} else {
assertNull(info.toString(), info.getId());
}
```
?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] s1monw commented on a change in pull request #1434:
LUCENE-9324: Add an ID to SegmentCommitInfo
Posted by GitBox <gi...@apache.org>.
s1monw commented on a change in pull request #1434: LUCENE-9324: Add an ID to SegmentCommitInfo
URL: https://github.com/apache/lucene-solr/pull/1434#discussion_r410370754
##########
File path: lucene/backward-codecs/src/test/org/apache/lucene/index/TestBackwardsCompatibility.java
##########
@@ -1367,6 +1368,16 @@ public void testIndexCreatedVersion() throws IOException {
}
}
+ public void testSegmentCommitInfoId() throws IOException {
+ for (String name : oldNames) {
+ Directory dir = oldIndexDirs.get(name);
+ SegmentInfos infos = SegmentInfos.readLatestCommit(dir);
+ for (SegmentCommitInfo info : infos) {
+ assertNull(info.getId());
Review comment:
👍
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] s1monw commented on a change in pull request #1434:
LUCENE-9324: Add an ID to SegmentCommitInfo
Posted by GitBox <gi...@apache.org>.
s1monw commented on a change in pull request #1434: LUCENE-9324: Add an ID to SegmentCommitInfo
URL: https://github.com/apache/lucene-solr/pull/1434#discussion_r409578428
##########
File path: lucene/core/src/java/org/apache/lucene/index/IndexWriter.java
##########
@@ -3081,7 +3081,7 @@ private SegmentCommitInfo copySegmentAsIs(SegmentCommitInfo info, String segName
info.info.getUseCompoundFile(), info.info.getCodec(),
info.info.getDiagnostics(), info.info.getId(), info.info.getAttributes(), info.info.getIndexSort());
SegmentCommitInfo newInfoPerCommit = new SegmentCommitInfo(newInfo, info.getDelCount(), info.getSoftDelCount(), info.getDelGen(),
- info.getFieldInfosGen(), info.getDocValuesGen());
+ info.getFieldInfosGen(), info.getDocValuesGen(), info.getId());
Review comment:
we do share the `info.info.getId()` here as well so I think we should be consistent?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jpountz commented on a change in pull request #1434:
LUCENE-9324: Add an ID to SegmentCommitInfo
Posted by GitBox <gi...@apache.org>.
jpountz commented on a change in pull request #1434: LUCENE-9324: Add an ID to SegmentCommitInfo
URL: https://github.com/apache/lucene-solr/pull/1434#discussion_r409795498
##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentCommitInfo.java
##########
@@ -79,8 +85,7 @@
/**
* Sole constructor.
- *
- * @param info
+ * @param info
Review comment:
nit:
```suggestion
* @param info
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jpountz commented on a change in pull request #1434:
LUCENE-9324: Add an ID to SegmentCommitInfo
Posted by GitBox <gi...@apache.org>.
jpountz commented on a change in pull request #1434: LUCENE-9324: Add an ID to SegmentCommitInfo
URL: https://github.com/apache/lucene-solr/pull/1434#discussion_r409804272
##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentInfos.java
##########
@@ -374,7 +376,15 @@ public static final SegmentInfos readCommit(Directory directory, ChecksumIndexIn
if (softDelCount + delCount > info.maxDoc()) {
throw new CorruptIndexException("invalid deletion count: " + softDelCount + delCount + " vs maxDoc=" + info.maxDoc(), input);
}
- SegmentCommitInfo siPerCommit = new SegmentCommitInfo(info, delCount, softDelCount, delGen, fieldInfosGen, dvGen);
+ final byte[] sciId;
+ if (format > VERSION_74) {
+ sciId = new byte[StringHelper.ID_LENGTH];
+ input.readBytes(sciId, 0, sciId.length);
+ } else {
+ sciId = infos.id;
+ // NOCOMMIT can we do this? it would at least give us consistent BWC but we can't identify the same SCI in different commits
Review comment:
When we introduced SegmentInfos#getId, we returned `null` as an ID for older segments. This is probably a safer option here as well, as callers can fall back to whatever behavior makes sense for them such as using a strong hash of the commit files as an ID, or re-downloading all files of the commit all the time and giving up incrementality?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] mikemccand commented on a change in pull request
#1434: LUCENE-9324: Add an ID to SegmentCommitInfo
Posted by GitBox <gi...@apache.org>.
mikemccand commented on a change in pull request #1434: LUCENE-9324: Add an ID to SegmentCommitInfo
URL: https://github.com/apache/lucene-solr/pull/1434#discussion_r409553076
##########
File path: lucene/core/src/java/org/apache/lucene/index/IndexWriter.java
##########
@@ -3081,7 +3081,7 @@ private SegmentCommitInfo copySegmentAsIs(SegmentCommitInfo info, String segName
info.info.getUseCompoundFile(), info.info.getCodec(),
info.info.getDiagnostics(), info.info.getId(), info.info.getAttributes(), info.info.getIndexSort());
SegmentCommitInfo newInfoPerCommit = new SegmentCommitInfo(newInfo, info.getDelCount(), info.getSoftDelCount(), info.getDelGen(),
- info.getFieldInfosGen(), info.getDocValuesGen());
+ info.getFieldInfosGen(), info.getDocValuesGen(), info.getId());
Review comment:
This happens during `IndexWriter.addIndexes(Directory[])` right? I wonder whether we should give a new id instead of reusing the old one? E.g. the segment (likely) now has a new name, and is in a different `Directory`, and is copied/forked from a prior segment, so maybe it should get a new `id`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] s1monw commented on issue #1434: LUCENE-9324: Add an
ID to SegmentCommitInfo
Posted by GitBox <gi...@apache.org>.
s1monw commented on issue #1434: LUCENE-9324: Add an ID to SegmentCommitInfo
URL: https://github.com/apache/lucene-solr/pull/1434#issuecomment-615216546
@mikemccand @jpountz I pushed new commits that preserves the null value as much as necessary until the segment actually changes. This way we will be able to fall back to different methods to figure out if segments are the _same_. please take another look
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] s1monw commented on a change in pull request #1434:
LUCENE-9324: Add an ID to SegmentCommitInfo
Posted by GitBox <gi...@apache.org>.
s1monw commented on a change in pull request #1434: LUCENE-9324: Add an ID to SegmentCommitInfo
URL: https://github.com/apache/lucene-solr/pull/1434#discussion_r410370849
##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentInfos.java
##########
@@ -374,7 +376,18 @@ public static final SegmentInfos readCommit(Directory directory, ChecksumIndexIn
if (softDelCount + delCount > info.maxDoc()) {
throw new CorruptIndexException("invalid deletion count: " + softDelCount + delCount + " vs maxDoc=" + info.maxDoc(), input);
}
- SegmentCommitInfo siPerCommit = new SegmentCommitInfo(info, delCount, softDelCount, delGen, fieldInfosGen, dvGen);
+ final byte[] sciId;
+ if (format > VERSION_74 ) {
+ if (input.readByte() == 1) {
Review comment:
👍
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] s1monw commented on a change in pull request #1434:
LUCENE-9324: Add an ID to SegmentCommitInfo
Posted by GitBox <gi...@apache.org>.
s1monw commented on a change in pull request #1434: LUCENE-9324: Add an ID to SegmentCommitInfo
URL: https://github.com/apache/lucene-solr/pull/1434#discussion_r409839154
##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentInfos.java
##########
@@ -374,7 +376,15 @@ public static final SegmentInfos readCommit(Directory directory, ChecksumIndexIn
if (softDelCount + delCount > info.maxDoc()) {
throw new CorruptIndexException("invalid deletion count: " + softDelCount + delCount + " vs maxDoc=" + info.maxDoc(), input);
}
- SegmentCommitInfo siPerCommit = new SegmentCommitInfo(info, delCount, softDelCount, delGen, fieldInfosGen, dvGen);
+ final byte[] sciId;
+ if (format > VERSION_74) {
+ sciId = new byte[StringHelper.ID_LENGTH];
+ input.readBytes(sciId, 0, sciId.length);
+ } else {
+ sciId = infos.id;
+ // NOCOMMIT can we do this? it would at least give us consistent BWC but we can't identify the same SCI in different commits
Review comment:
I pushed a new commit https://github.com/apache/lucene-solr/pull/1434/commits/f0a72f82bb17bd2582799aa25514ef764e012570 to address this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] mikemccand commented on a change in pull request
#1434: LUCENE-9324: Add an ID to SegmentCommitInfo
Posted by GitBox <gi...@apache.org>.
mikemccand commented on a change in pull request #1434: LUCENE-9324: Add an ID to SegmentCommitInfo
URL: https://github.com/apache/lucene-solr/pull/1434#discussion_r410387793
##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentCommitInfo.java
##########
@@ -77,9 +83,11 @@
// this is never written to/read from the Directory
private long bufferedDeletesGen = -1;
+ // is set once any of the generations has been advanced.
+ private boolean hasAdvanced;
Review comment:
We can remove this now?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] s1monw commented on a change in pull request #1434:
LUCENE-9324: Add an ID to SegmentCommitInfo
Posted by GitBox <gi...@apache.org>.
s1monw commented on a change in pull request #1434: LUCENE-9324: Add an ID to SegmentCommitInfo
URL: https://github.com/apache/lucene-solr/pull/1434#discussion_r409577804
##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentInfos.java
##########
@@ -374,7 +376,15 @@ public static final SegmentInfos readCommit(Directory directory, ChecksumIndexIn
if (softDelCount + delCount > info.maxDoc()) {
throw new CorruptIndexException("invalid deletion count: " + softDelCount + delCount + " vs maxDoc=" + info.maxDoc(), input);
}
- SegmentCommitInfo siPerCommit = new SegmentCommitInfo(info, delCount, softDelCount, delGen, fieldInfosGen, dvGen);
+ final byte[] sciId;
+ if (format > VERSION_74) {
+ sciId = new byte[StringHelper.ID_LENGTH];
+ input.readBytes(sciId, 0, sciId.length);
+ } else {
+ sciId = infos.id;
+ // NOCOMMIT can we do this? it would at least give us consistent BWC but we can't identify the same SCI in different commits
Review comment:
I don't think we should use info.getId() since that would mean that the same SegmentInfo instances are treated the same even if two IW made changes to it and it's generations. The way we have it now it's only considered the same if the overall commit is the same which is good i guess?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jpountz commented on a change in pull request #1434:
LUCENE-9324: Add an ID to SegmentCommitInfo
Posted by GitBox <gi...@apache.org>.
jpountz commented on a change in pull request #1434: LUCENE-9324: Add an ID to SegmentCommitInfo
URL: https://github.com/apache/lucene-solr/pull/1434#discussion_r409844046
##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentInfos.java
##########
@@ -374,7 +376,15 @@ public static final SegmentInfos readCommit(Directory directory, ChecksumIndexIn
if (softDelCount + delCount > info.maxDoc()) {
throw new CorruptIndexException("invalid deletion count: " + softDelCount + delCount + " vs maxDoc=" + info.maxDoc(), input);
}
- SegmentCommitInfo siPerCommit = new SegmentCommitInfo(info, delCount, softDelCount, delGen, fieldInfosGen, dvGen);
+ final byte[] sciId;
+ if (format > VERSION_74) {
+ sciId = new byte[StringHelper.ID_LENGTH];
+ input.readBytes(sciId, 0, sciId.length);
+ } else {
+ sciId = infos.id;
+ // NOCOMMIT can we do this? it would at least give us consistent BWC but we can't identify the same SCI in different commits
Review comment:
I don't understand the idea of stealing the parent ID, wouldn't it cause Lucene to consider commits equal when they are not, which would be a much worse problem than considering commits different when they are equal?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] s1monw commented on a change in pull request #1434:
LUCENE-9324: Add an ID to SegmentCommitInfo
Posted by GitBox <gi...@apache.org>.
s1monw commented on a change in pull request #1434: LUCENE-9324: Add an ID to SegmentCommitInfo
URL: https://github.com/apache/lucene-solr/pull/1434#discussion_r409575955
##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentCommitInfo.java
##########
@@ -388,4 +399,17 @@ public SegmentCommitInfo clone() {
final int getDelCount(boolean includeSoftDeletes) {
return includeSoftDeletes ? getDelCount() + getSoftDelCount() : getDelCount();
}
+
+ private void generationAdvanced() {
+ sizeInBytes = -1;
+ id = null;
+ }
+
+ public byte[] getId() {
+ if (id == null) {
+ // we advanced a generation - need to generate a new ID
+ id = StringHelper.randomId();
Review comment:
yeah good question, as far as I can tell we never read or write any of the member vars on this class unless we hold a lock that protects it. But it's a mess so I guess we should. Yet, if we do that we should make every method on this class synced no?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] s1monw commented on issue #1434: LUCENE-9324: Add an
ID to SegmentCommitInfo
Posted by GitBox <gi...@apache.org>.
s1monw commented on issue #1434: LUCENE-9324: Add an ID to SegmentCommitInfo
URL: https://github.com/apache/lucene-solr/pull/1434#issuecomment-614643954
Note: there is still a NOCOMMIT in this pr regarding BWC. It's just an idea we can build on or even move to using `null` as an indicator that there is no id.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jpountz commented on a change in pull request #1434:
LUCENE-9324: Add an ID to SegmentCommitInfo
Posted by GitBox <gi...@apache.org>.
jpountz commented on a change in pull request #1434: LUCENE-9324: Add an ID to SegmentCommitInfo
URL: https://github.com/apache/lucene-solr/pull/1434#discussion_r410184987
##########
File path: lucene/backward-codecs/src/test/org/apache/lucene/index/TestBackwardsCompatibility.java
##########
@@ -830,8 +829,9 @@ public void testAddOldIndexes() throws IOException {
IndexWriter w = new IndexWriter(targetDir, newIndexWriterConfig(new MockAnalyzer(random())));
w.addIndexes(oldDir);
w.close();
- targetDir.close();
+ SegmentInfos si = SegmentInfos.readLatestCommit(targetDir);
+ assertEquals("none of the segments should have been upgraded", 0, si.asList().stream().filter(sci -> sci.getId() != null).count());
Review comment:
nit: turn it in such a way that it would return a non-null SCI to make it easier to debug in case it fails? E.g.
```
assertNull("none of the segments should have been upgraded", si.asList().stream().filter(sci -> sci.getId() != null).findAny().orElse(null));
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] s1monw commented on a change in pull request #1434:
LUCENE-9324: Add an ID to SegmentCommitInfo
Posted by GitBox <gi...@apache.org>.
s1monw commented on a change in pull request #1434: LUCENE-9324: Add an ID to SegmentCommitInfo
URL: https://github.com/apache/lucene-solr/pull/1434#discussion_r409812796
##########
File path: lucene/core/src/java/org/apache/lucene/index/SegmentInfos.java
##########
@@ -374,7 +376,15 @@ public static final SegmentInfos readCommit(Directory directory, ChecksumIndexIn
if (softDelCount + delCount > info.maxDoc()) {
throw new CorruptIndexException("invalid deletion count: " + softDelCount + delCount + " vs maxDoc=" + info.maxDoc(), input);
}
- SegmentCommitInfo siPerCommit = new SegmentCommitInfo(info, delCount, softDelCount, delGen, fieldInfosGen, dvGen);
+ final byte[] sciId;
+ if (format > VERSION_74) {
+ sciId = new byte[StringHelper.ID_LENGTH];
+ input.readBytes(sciId, 0, sciId.length);
+ } else {
+ sciId = infos.id;
+ // NOCOMMIT can we do this? it would at least give us consistent BWC but we can't identify the same SCI in different commits
Review comment:
I think this case is a bit more complicated due to the changing nature of this ID. For each change (DV, llive docs, fields) we need to move to a new ID. Should we then just accept null and create a new ID once it changes or should we stick with `null` on these segments until they are written first time? Introducing `null` requires quite some changes in how we handle this which we can do, for sure. I still wonder if we can get away with stealing the _parent_ ID and have a smooth upgrade path.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org