You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/02/01 07:46:18 UTC
[GitHub] [iceberg] amogh-jahagirdar opened a new pull request #4019: Core: Add SnapshotRef to API and add parser and serializer
amogh-jahagirdar opened a new pull request #4019:
URL: https://github.com/apache/iceberg/pull/4019
Co-authored-by: wangzeyu <12...@qq.com>
Co-authored-by: Jack Ye <yz...@amazon.com>
This is a smaller PR as part of breaking up the changes in https://github.com/apache/iceberg/pull/3883/files. This change includes the SnapshotRef entity in the Iceberg API as well as a SnapshotReferenceParser utility class to be used when serializing and deserializing as part of TableMetadata parsing/serializing.
TableMetadata parsing/serializing of the reference will be a part of the PR which includes TableMetadata changes for branching/tagging, as we will need the metadata APIs defined. For this PR the focus is the SnapshotRef entity, the serializing/deserializing utility class and tests.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] amogh-jahagirdar commented on a change in pull request #4019: Core: Add SnapshotRef entity to API
Posted by GitBox <gi...@apache.org>.
amogh-jahagirdar commented on a change in pull request #4019:
URL: https://github.com/apache/iceberg/pull/4019#discussion_r797882834
##########
File path: api/src/main/java/org/apache/iceberg/SnapshotRef.java
##########
@@ -0,0 +1,177 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg;
+
+import java.io.Serializable;
+import java.util.Objects;
+import org.apache.iceberg.relocated.com.google.common.base.MoreObjects;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class SnapshotRef implements Serializable {
Review comment:
Ah yes, my bad I meant to include it. I have added the test 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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4019: Core: Add SnapshotRef entity to API
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4019:
URL: https://github.com/apache/iceberg/pull/4019#discussion_r798059886
##########
File path: api/src/main/java/org/apache/iceberg/SnapshotRef.java
##########
@@ -0,0 +1,177 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg;
+
+import java.io.Serializable;
+import java.util.Objects;
+import org.apache.iceberg.relocated.com.google.common.base.MoreObjects;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class SnapshotRef implements Serializable {
Review comment:
I agree with @jackye1995 here. Immutables doesn't seem like a good fit. Maybe if we were starting the project that way, but I doubt it is worth rewriting so much existing code. And if we start using it for new classes, then we end up with slight differences without much benefit.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4019: Core: Add SnapshotRef entity to API
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4019:
URL: https://github.com/apache/iceberg/pull/4019#discussion_r798036900
##########
File path: api/src/main/java/org/apache/iceberg/SnapshotRef.java
##########
@@ -0,0 +1,177 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg;
+
+import java.io.Serializable;
+import java.util.Objects;
+import org.apache.iceberg.relocated.com.google.common.base.MoreObjects;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class SnapshotRef implements Serializable {
Review comment:
I understand, but with that we are then just saving a few lines of code. I don't see much additional benefit, and we break a few code patterns in the codebase including:
1. it uses `getSomeValue` instead of `someValue` as method name for getters
2. we cannot have flexible logic inside the builder to check for specific `ValidationException` and has to do it outside the generated builder
3. we have separated API module code patterns with some using Immutable and some do not
that's why I was asking for what are the other benefits.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4019: Core: Add SnapshotRef entity to API
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4019:
URL: https://github.com/apache/iceberg/pull/4019#discussion_r798060484
##########
File path: api/src/main/java/org/apache/iceberg/SnapshotRef.java
##########
@@ -0,0 +1,177 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg;
+
+import java.io.Serializable;
+import java.util.Objects;
+import org.apache.iceberg.relocated.com.google.common.base.MoreObjects;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class SnapshotRef implements Serializable {
+
+ public static final String MAIN_BRANCH = "main";
+
+ private final long snapshotId;
+ private final SnapshotRefType type;
+ private final Integer minSnapshotsToKeep;
+ private final Long maxSnapshotAgeMs;
+ private final Long maxRefAgeMs;
+
+ private SnapshotRef(
+ long snapshotId,
+ SnapshotRefType type,
+ Integer minSnapshotsToKeep,
+ Long maxSnapshotAgeMs,
+ Long maxRefAgeMs) {
+ this.snapshotId = snapshotId;
+ this.type = type;
+ this.minSnapshotsToKeep = minSnapshotsToKeep;
+ this.maxSnapshotAgeMs = maxSnapshotAgeMs;
+ this.maxRefAgeMs = maxRefAgeMs;
+ }
+
+ public long snapshotId() {
+ return snapshotId;
+ }
+
+ public SnapshotRefType type() {
+ return type;
+ }
+
+ public Integer minSnapshotsToKeep() {
+ return minSnapshotsToKeep;
+ }
+
+ public Long maxSnapshotAgeMs() {
+ return maxSnapshotAgeMs;
+ }
+
+ public Long maxRefAgeMs() {
+ return maxRefAgeMs;
+ }
+
+ public static Builder tagBuilder(long snapshotId) {
+ return builderFor(snapshotId, SnapshotRefType.TAG);
+ }
+
+ public static Builder branchBuilder(long snapshotId) {
+ return builderFor(snapshotId, SnapshotRefType.BRANCH);
+ }
+
+ public static Builder builderFrom(SnapshotRef ref) {
+ return new Builder(ref.type(), ref.snapshotId())
+ .minSnapshotsToKeep(ref.minSnapshotsToKeep())
+ .maxSnapshotAgeMs(ref.maxSnapshotAgeMs())
+ .maxRefAgeMs(ref.maxRefAgeMs());
+ }
+
+ /**
+ * Creates a ref builder from the given ref and its properties but the ref will now point to the given snapshotId.
+ *
+ * @param ref Ref to build from
+ * @param snapshotId snapshotID to use.
+ * @return ref builder with the same retention properties as given ref, but the ref will point to the passed in id
+ */
+ public static Builder builderFrom(SnapshotRef ref, long snapshotId) {
+ return new Builder(ref.type(), snapshotId)
+ .minSnapshotsToKeep(ref.minSnapshotsToKeep())
+ .maxSnapshotAgeMs(ref.maxSnapshotAgeMs())
+ .maxRefAgeMs(ref.maxRefAgeMs());
+ }
+
+ public static Builder builderFor(long snapshotId, SnapshotRefType type) {
+ return new Builder(type, snapshotId);
+ }
+
+ public static class Builder {
+
+ private final SnapshotRefType type;
+ private final long snapshotId;
+ private Integer minSnapshotsToKeep;
+ private Long maxSnapshotAgeMs;
+ private Long maxRefAgeMs;
+
+ Builder(SnapshotRefType type, long snapshotId) {
+ Preconditions.checkArgument(type != null, "Snapshot reference type must not be null");
+ this.type = type;
+ this.snapshotId = snapshotId;
+ }
+
+ public Builder minSnapshotsToKeep(Integer value) {
+ Preconditions.checkArgument(value == null || !type.equals(SnapshotRefType.TAG),
+ "Tags do not support setting minSnapshotsToKeep");
+ Preconditions.checkArgument(value == null || value > 0,
+ "Min snapshots to keep must be greater than 0");
+ this.minSnapshotsToKeep = value;
+ return this;
+ }
+
+ public Builder maxSnapshotAgeMs(Long value) {
+ Preconditions.checkArgument(value == null || !type.equals(SnapshotRefType.TAG),
+ "Tags do not support setting maxSnapshotAgeMs");
+ Preconditions.checkArgument(value == null || value > 0,
+ "Max snapshot age must be greater than 0");
Review comment:
0 ms?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] amogh-jahagirdar commented on a change in pull request #4019: Core: Add SnapshotRef entity to API
Posted by GitBox <gi...@apache.org>.
amogh-jahagirdar commented on a change in pull request #4019:
URL: https://github.com/apache/iceberg/pull/4019#discussion_r797882834
##########
File path: api/src/main/java/org/apache/iceberg/SnapshotRef.java
##########
@@ -0,0 +1,177 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg;
+
+import java.io.Serializable;
+import java.util.Objects;
+import org.apache.iceberg.relocated.com.google.common.base.MoreObjects;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class SnapshotRef implements Serializable {
Review comment:
Ah yes, my bad I meant to include it.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on pull request #4019: Core: Add SnapshotRef entity to API
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on pull request #4019:
URL: https://github.com/apache/iceberg/pull/4019#issuecomment-1028457154
Thanks for the work, merged!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a change in pull request #4019: Core: Add SnapshotRef entity to API
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #4019:
URL: https://github.com/apache/iceberg/pull/4019#discussion_r797358512
##########
File path: api/src/main/java/org/apache/iceberg/SnapshotRef.java
##########
@@ -0,0 +1,177 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg;
+
+import java.io.Serializable;
+import java.util.Objects;
+import org.apache.iceberg.relocated.com.google.common.base.MoreObjects;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class SnapshotRef implements Serializable {
Review comment:
I think this would be a good example where we could use https://immutables.github.io/. The advantage would be that you don't have to manually write + test your builder and you can get JSON serialization for free. However, you can still use the `SnapshotReferenceParser` from #3883 and not rely in the immutables lib for JSON serialization.
Thoughts @amogh-jahagirdar?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] amogh-jahagirdar commented on a change in pull request #4019: Core: Add SnapshotRef entity to API and add snapshot ref serializer/deserializer utility class.
Posted by GitBox <gi...@apache.org>.
amogh-jahagirdar commented on a change in pull request #4019:
URL: https://github.com/apache/iceberg/pull/4019#discussion_r796330264
##########
File path: api/src/main/java/org/apache/iceberg/SnapshotRef.java
##########
@@ -0,0 +1,176 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg;
+
+import java.io.Serializable;
+import java.util.Objects;
+import org.apache.iceberg.relocated.com.google.common.base.MoreObjects;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class SnapshotRef implements Serializable {
+ public static final String MAIN_BRANCH = "main";
+
+ private final long snapshotId;
+ private final SnapshotRefType type;
+ private final Integer minSnapshotsToKeep;
+ private final Long maxSnapshotAgeMs;
+ private final Long maxRefAgeMs;
+
Review comment:
For updating retention logic we will need timestamps per SnapshotRef. I'm thinking we defer that for that PR, but let me know your thoughts!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4019: Core: Add SnapshotRef entity to API
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4019:
URL: https://github.com/apache/iceberg/pull/4019#discussion_r798062641
##########
File path: api/src/main/java/org/apache/iceberg/SnapshotRef.java
##########
@@ -0,0 +1,177 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg;
+
+import java.io.Serializable;
+import java.util.Objects;
+import org.apache.iceberg.relocated.com.google.common.base.MoreObjects;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class SnapshotRef implements Serializable {
+
+ public static final String MAIN_BRANCH = "main";
+
+ private final long snapshotId;
+ private final SnapshotRefType type;
+ private final Integer minSnapshotsToKeep;
+ private final Long maxSnapshotAgeMs;
+ private final Long maxRefAgeMs;
+
+ private SnapshotRef(
+ long snapshotId,
+ SnapshotRefType type,
+ Integer minSnapshotsToKeep,
+ Long maxSnapshotAgeMs,
+ Long maxRefAgeMs) {
+ this.snapshotId = snapshotId;
+ this.type = type;
+ this.minSnapshotsToKeep = minSnapshotsToKeep;
+ this.maxSnapshotAgeMs = maxSnapshotAgeMs;
+ this.maxRefAgeMs = maxRefAgeMs;
+ }
+
+ public long snapshotId() {
+ return snapshotId;
+ }
+
+ public SnapshotRefType type() {
+ return type;
+ }
+
+ public Integer minSnapshotsToKeep() {
+ return minSnapshotsToKeep;
+ }
+
+ public Long maxSnapshotAgeMs() {
+ return maxSnapshotAgeMs;
+ }
+
+ public Long maxRefAgeMs() {
+ return maxRefAgeMs;
+ }
+
+ public static Builder tagBuilder(long snapshotId) {
+ return builderFor(snapshotId, SnapshotRefType.TAG);
+ }
+
+ public static Builder branchBuilder(long snapshotId) {
+ return builderFor(snapshotId, SnapshotRefType.BRANCH);
+ }
+
+ public static Builder builderFrom(SnapshotRef ref) {
+ return new Builder(ref.type(), ref.snapshotId())
+ .minSnapshotsToKeep(ref.minSnapshotsToKeep())
+ .maxSnapshotAgeMs(ref.maxSnapshotAgeMs())
+ .maxRefAgeMs(ref.maxRefAgeMs());
+ }
+
+ /**
+ * Creates a ref builder from the given ref and its properties but the ref will now point to the given snapshotId.
+ *
+ * @param ref Ref to build from
+ * @param snapshotId snapshotID to use.
+ * @return ref builder with the same retention properties as given ref, but the ref will point to the passed in id
+ */
+ public static Builder builderFrom(SnapshotRef ref, long snapshotId) {
+ return new Builder(ref.type(), snapshotId)
+ .minSnapshotsToKeep(ref.minSnapshotsToKeep())
+ .maxSnapshotAgeMs(ref.maxSnapshotAgeMs())
+ .maxRefAgeMs(ref.maxRefAgeMs());
+ }
+
+ public static Builder builderFor(long snapshotId, SnapshotRefType type) {
+ return new Builder(type, snapshotId);
+ }
+
+ public static class Builder {
+
+ private final SnapshotRefType type;
+ private final long snapshotId;
+ private Integer minSnapshotsToKeep;
+ private Long maxSnapshotAgeMs;
+ private Long maxRefAgeMs;
+
+ Builder(SnapshotRefType type, long snapshotId) {
+ Preconditions.checkArgument(type != null, "Snapshot reference type must not be null");
+ this.type = type;
+ this.snapshotId = snapshotId;
+ }
+
+ public Builder minSnapshotsToKeep(Integer value) {
+ Preconditions.checkArgument(value == null || !type.equals(SnapshotRefType.TAG),
+ "Tags do not support setting minSnapshotsToKeep");
+ Preconditions.checkArgument(value == null || value > 0,
+ "Min snapshots to keep must be greater than 0");
+ this.minSnapshotsToKeep = value;
+ return this;
+ }
+
+ public Builder maxSnapshotAgeMs(Long value) {
+ Preconditions.checkArgument(value == null || !type.equals(SnapshotRefType.TAG),
+ "Tags do not support setting maxSnapshotAgeMs");
+ Preconditions.checkArgument(value == null || value > 0,
+ "Max snapshot age must be greater than 0");
+ this.maxSnapshotAgeMs = value;
+ return this;
+ }
+
+ public Builder maxRefAgeMs(Long value) {
+ Preconditions.checkArgument(value == null || value > 0, "Max reference age must be greater than 0");
+ this.maxRefAgeMs = value;
+ return this;
+ }
+
+ public SnapshotRef build() {
+ return new SnapshotRef(snapshotId, type, minSnapshotsToKeep, maxSnapshotAgeMs, maxRefAgeMs);
+ }
+ }
+
+ @Override
+ public boolean equals(Object o) {
Review comment:
Since the tests all check the individual values, `equals` and `hashCode` implementations aren't really needed. Are you planning on using them later?
The trouble with doing this right now is that you have to decide on what equality means for a `SnapshotRef`. This defaults to checking all fields, but it could be that it makes sense to use the name and the snapshot at some point in the future because the other 3 fields are metadata.
Normally, I recommend not making this decision until we need to. If you want to keep this implementation, I'm okay with that. It's up to you.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 merged pull request #4019: Core: Add SnapshotRef entity to API
Posted by GitBox <gi...@apache.org>.
jackye1995 merged pull request #4019:
URL: https://github.com/apache/iceberg/pull/4019
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4019: Core: Add SnapshotRef entity to API
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4019:
URL: https://github.com/apache/iceberg/pull/4019#discussion_r797937083
##########
File path: api/src/main/java/org/apache/iceberg/SnapshotRef.java
##########
@@ -0,0 +1,177 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg;
+
+import java.io.Serializable;
+import java.util.Objects;
+import org.apache.iceberg.relocated.com.google.common.base.MoreObjects;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class SnapshotRef implements Serializable {
Review comment:
borderline here, more on the -1 side.
I think we already had a few discussions around using immutable, and we came to the conclusion that it does not help with our JSON serialization, because for `TableMeadata` related objects it still needs to be integrated with the parser, and for `ScanTask` related objects the generated serializer and deserializer cannot be leveraged by the systems that need to use it including Presto and Trino.
So for consistency with other objects in the API module I would say let's just keep the dependency tree clean and not use it. But I am definitely open to it if there are other benefits.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a change in pull request #4019: Core: Add SnapshotRef entity to API
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #4019:
URL: https://github.com/apache/iceberg/pull/4019#discussion_r797968636
##########
File path: api/src/main/java/org/apache/iceberg/SnapshotRef.java
##########
@@ -0,0 +1,177 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg;
+
+import java.io.Serializable;
+import java.util.Objects;
+import org.apache.iceberg.relocated.com.google.common.base.MoreObjects;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class SnapshotRef implements Serializable {
Review comment:
Right, my suggestion was rather meant to use Immutables without having to use Immutables Json serialization functionality as there is already the parser available that takes care of that.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 edited a comment on pull request #4019: Core: Add SnapshotRef entity to API
Posted by GitBox <gi...@apache.org>.
jackye1995 edited a comment on pull request #4019:
URL: https://github.com/apache/iceberg/pull/4019#issuecomment-1028457154
Thanks for the work, merged! And thanks for the review @rdblue @nastra
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a change in pull request #4019: Core: Add SnapshotRef entity to API
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #4019:
URL: https://github.com/apache/iceberg/pull/4019#discussion_r798294220
##########
File path: api/src/main/java/org/apache/iceberg/SnapshotRef.java
##########
@@ -0,0 +1,177 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg;
+
+import java.io.Serializable;
+import java.util.Objects;
+import org.apache.iceberg.relocated.com.google.common.base.MoreObjects;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class SnapshotRef implements Serializable {
Review comment:
> I understand, but with that we are then just saving a few lines of code. I don't see much additional benefit, and we break a few code patterns in the codebase including:
>
> 1. it uses `getSomeValue` instead of `someValue` as method name for getters
> 2. we cannot have flexible logic inside the builder to check for specific `ValidationException` and has to do it outside the generated builder
> 3. we have separated API module code patterns with some using Immutable and some do not
>
> that's why I was asking for what are the other benefits.
Just for clarification as there seem to be some misinterpretation of how Immutables work (and maybe I did a terrible job in explaining it previously):
IMO the biggest benefit is that you get truly immutable objects that are always in a consistent state, thus allowing you to implicitly enforce good engineering practices through those. All the other things (generated builders, JSON support, ...) are additional bonuses that you don't have to use if not needed
1. you can choose any prefix for those methods, so it can be with or without `get`
2. you can still have that flexibility. At the time when I proposed the first PR with Immutables it wasn't clear that this is something that we wanted to have but it doesn't mean that you can't have that with Immutables
3. I'm not sure why it would matter that much if some classes would use Immutables and some wouldn't. As long as you're using the existing API to construct those instances, it shouldn't make a difference for users of those classes. You can have static methods and hide the `ImmutableXyzBuilder` behind it like it's being done in https://github.com/apache/iceberg/blob/063281ab0a7ab72f2927aa19376625f3ff55be97/api/src/main/java/org/apache/iceberg/catalog/Namespace.java#L37-L43.
4. Introducing Immutables doesn't mean that we would have to rewrite everything. We should only do so where it makes sense and where we'd like to make sure things are truly immutable
I hope that helps a bit. Anyway, I didn't want to hijack this PR with a discussion around Immutables but rather just propose something that's helpful, so let's move this discussion to a different place :)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] amogh-jahagirdar commented on a change in pull request #4019: Core: Add SnapshotRef entity to API and add snapshot ref serializer/deserializer utility class.
Posted by GitBox <gi...@apache.org>.
amogh-jahagirdar commented on a change in pull request #4019:
URL: https://github.com/apache/iceberg/pull/4019#discussion_r796330264
##########
File path: api/src/main/java/org/apache/iceberg/SnapshotRef.java
##########
@@ -0,0 +1,176 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg;
+
+import java.io.Serializable;
+import java.util.Objects;
+import org.apache.iceberg.relocated.com.google.common.base.MoreObjects;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class SnapshotRef implements Serializable {
+ public static final String MAIN_BRANCH = "main";
+
+ private final long snapshotId;
+ private final SnapshotRefType type;
+ private final Integer minSnapshotsToKeep;
+ private final Long maxSnapshotAgeMs;
+ private final Long maxRefAgeMs;
+
Review comment:
For updating retention logic I think we will need a timestamp field per SnapshotRef. I'm thinking we defer that for that PR, but let me know your thoughts!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] amogh-jahagirdar commented on a change in pull request #4019: Core: Add SnapshotRef entity to API and add snapshot ref serializer/deserializer utility class.
Posted by GitBox <gi...@apache.org>.
amogh-jahagirdar commented on a change in pull request #4019:
URL: https://github.com/apache/iceberg/pull/4019#discussion_r796330264
##########
File path: api/src/main/java/org/apache/iceberg/SnapshotRef.java
##########
@@ -0,0 +1,176 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg;
+
+import java.io.Serializable;
+import java.util.Objects;
+import org.apache.iceberg.relocated.com.google.common.base.MoreObjects;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class SnapshotRef implements Serializable {
+ public static final String MAIN_BRANCH = "main";
+
+ private final long snapshotId;
+ private final SnapshotRefType type;
+ private final Integer minSnapshotsToKeep;
+ private final Long maxSnapshotAgeMs;
+ private final Long maxRefAgeMs;
+
Review comment:
For updating retention logic we will need timestamps per SnapshotRef. I'm thinking we defer that for another PR.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a change in pull request #4019: Core: Add SnapshotRef entity to API
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #4019:
URL: https://github.com/apache/iceberg/pull/4019#discussion_r797354350
##########
File path: api/src/main/java/org/apache/iceberg/SnapshotRef.java
##########
@@ -0,0 +1,177 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg;
+
+import java.io.Serializable;
+import java.util.Objects;
+import org.apache.iceberg.relocated.com.google.common.base.MoreObjects;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class SnapshotRef implements Serializable {
Review comment:
it would be good to include the testclass as well from https://github.com/apache/iceberg/pull/3883 in this PR
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org