You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by GitBox <gi...@apache.org> on 2021/04/09 05:19:37 UTC
[GitHub] [james-project] chibenwa opened a new pull request #377: JAMES-3557 */changes: Fail explicitly when too much entries on a single change
chibenwa opened a new pull request #377:
URL: https://github.com/apache/james-project/pull/377
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] Arsnael commented on pull request #377: JAMES-3557 */changes: Fail explicitly when too much entries on a single change
Posted by GitBox <gi...@apache.org>.
Arsnael commented on pull request #377:
URL: https://github.com/apache/james-project/pull/377#issuecomment-817525039
> No not really. If the chunck is too large, the chunk is too large. The client should use a bigger /changes then.
What is the point of having `hasMoreChanges` field and mechanic from the spec then if the chunk is too large we return an exception anyway? I'm trying to understand the logic behind this choice (and not about the technical limitations we might have from our code), which seems odd to me sorry
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa merged pull request #377: JAMES-3557 */changes: Fail explicitly when too much entries on a single change
Posted by GitBox <gi...@apache.org>.
chibenwa merged pull request #377:
URL: https://github.com/apache/james-project/pull/377
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on pull request #377: JAMES-3557 */changes: Fail explicitly when too much entries on a single change
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #377:
URL: https://github.com/apache/james-project/pull/377#issuecomment-817476106
> This looks like just a workaround, is there no way to fix this issue to turn into the correct behavior?
No not really. If the chunck is too large, the chunk is too large. The client should use a bigger /changes then.
An alternative would be to , upon insert, generate several smaller changes instead than one big. But I don't like this approach much, to be honnest.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] Arsnael commented on pull request #377: JAMES-3557 */changes: Fail explicitly when too much entries on a single change
Posted by GitBox <gi...@apache.org>.
Arsnael commented on pull request #377:
URL: https://github.com/apache/james-project/pull/377#issuecomment-822221858
> What is the point of having `hasMoreChanges` field and mechanic from the spec then if the chunk is too large we return an exception anyway? I'm trying to understand the logic behind this choice (and not about the technical limitations we might have from our code), which seems odd to me sorry
Still waiting an answer regarding this concern?
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] Arsnael commented on pull request #377: JAMES-3557 */changes: Fail explicitly when too much entries on a single change
Posted by GitBox <gi...@apache.org>.
Arsnael commented on pull request #377:
URL: https://github.com/apache/james-project/pull/377#issuecomment-817527550
```
02:30:07,088 [INFO] Running org.apache.james.mailbox.cassandra.CassandraMessageIdManagerStorageTest
02:31:24,808 [ERROR] Tests run: 60, Failures: 0, Errors: 60, Skipped: 0, Time elapsed: 77.705 s <<< FAILURE! - in org.apache.james.mailbox.cassandra.CassandraMessageIdManagerStorageTest
02:31:24,809 [ERROR] setInMailboxesShouldAllowCopyingMessageFromReadOnlySharedMailbox Time elapsed: 0.594 s <<< ERROR!
java.lang.NoClassDefFoundError: org/apache/james/events/EventBusTestFixture
at org.apache.james.mailbox.cassandra.CassandraMessageIdManagerStorageTest.createTestingData(CassandraMessageIdManagerStorageTest.java:41)
Caused by: java.lang.ClassNotFoundException: org.apache.james.events.EventBusTestFixture
at org.apache.james.mailbox.cassandra.CassandraMessageIdManagerStorageTest.createTestingData(CassandraMessageIdManagerStorageTest.java:41)
```
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] chibenwa commented on pull request #377: JAMES-3557 */changes: Fail explicitly when too much entries on a single change
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #377:
URL: https://github.com/apache/james-project/pull/377#issuecomment-822241552
Say we mutate 5845839345345345 messages at the same time.
We will store the changes in a single row.
We should return all the changes.
But if the maxChanges parameter is too low (say 256) then we can not return 5845839345345345 ids as it would exceed the client maxChanges. So we don't.
Today behaviour is to stay stuck (ouch!). Clients not aware of the bug will thus not be able to resync themselves.
With that changes, we return `I can not sync given your max changes`. The client can then resync with a higher max changes, or do a full resync...
> What is the point of having hasMoreChanges field and mechanic from the spec then if the chunk is too large we return an exception anyway?
It is for when you page updates (eg 4098 changes but I resync 256 at a time - wich works if we do not have crazy large records).
> I'm trying to understand the logic behind this choice (and not about the technical limitations we might have from our code)
Fair.
Ideally we could "chunk changes" by batches of say 256 upon saving them in the changelog so that no changelog entry exceeds a decent maxChanges value.
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
[GitHub] [james-project] LanKhuat commented on a change in pull request #377: JAMES-3557 */changes: Fail explicitly when too much entries on a single change
Posted by GitBox <gi...@apache.org>.
LanKhuat commented on a change in pull request #377:
URL: https://github.com/apache/james-project/pull/377#discussion_r610476045
##########
File path: server/data/data-jmap/src/test/java/org/apache/james/jmap/api/change/MailboxChangeRepositoryContract.java
##########
@@ -376,7 +376,7 @@ default void getChangesShouldNotReturnMoreThanMaxChanges() {
}
@Test
- default void getChangesShouldReturnEmptyWhenNumberOfChangesExceedMaxChanges() {
+ default void getChangesShouldReturnThrowWhenNumberOfChangesExceedMaxChanges() {
Review comment:
getChangesShouldThrow...
##########
File path: server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/EmailChangesMethodContract.scala
##########
@@ -197,6 +197,110 @@ trait EmailChangesMethodContract {
}
}
+ @Test
+ def shouldDailWithCannotCalculateChangesWhenSingleChangeIsTooLarge(server: GuiceJamesServer): Unit = {
+ val mailboxProbe: MailboxProbeImpl = server.getProbe(classOf[MailboxProbeImpl])
+ val path: MailboxPath = MailboxPath.forUser(BOB, "mailbox1")
+
+ mailboxProbe.createMailbox(path)
+
+ val message: Message = Message.Builder
+ .of
+ .setSubject("test")
+ .setBody("testmail", StandardCharsets.UTF_8)
+ .build
+ val messageId1: MessageId = mailboxProbe.appendMessage(BOB.asString(), path, AppendCommand.from(message)).getMessageId
+ val messageId2: MessageId = mailboxProbe.appendMessage(BOB.asString(), path, AppendCommand.from(message)).getMessageId
+ val messageId3: MessageId = mailboxProbe.appendMessage(BOB.asString(), path, AppendCommand.from(message)).getMessageId
+ val messageId4: MessageId = mailboxProbe.appendMessage(BOB.asString(), path, AppendCommand.from(message)).getMessageId
+ val messageId5: MessageId = mailboxProbe.appendMessage(BOB.asString(), path, AppendCommand.from(message)).getMessageId
+ val messageId6: MessageId = mailboxProbe.appendMessage(BOB.asString(), path, AppendCommand.from(message)).getMessageId
+
+ val oldState: State = waitForNextState(server, AccountId.fromUsername(BOB), State.INITIAL)
Review comment:
Did you test this on the distributed environment? You append 6 messages so it should have 6 more states after the INITIAL one.
##########
File path: server/protocols/jmap-rfc-8621-integration-tests/jmap-rfc-8621-integration-tests-common/src/main/scala/org/apache/james/jmap/rfc8621/contract/EmailChangesMethodContract.scala
##########
@@ -168,9 +168,9 @@ trait EmailChangesMethodContract {
val response = `given`
.header(ACCEPT.toString, ACCEPT_RFC8621_VERSION_HEADER)
.body(request)
- .when
+ .when
.post
- .`then`
+ .`then`
Review comment:
indent
--
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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org