Page MenuHomePhabricator
Differential D7309

[web] Pop up a pin/unpin modal when the tooltip icon is clicked
ClosedPublic
Actions

Authored by rohan on Apr 4 2023, 7:20 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 2, 7:24 PM
Unknown Object (File)
Tue, Apr 1, 8:28 AM
Unknown Object (File)
Tue, Apr 1, 8:28 AM
Unknown Object (File)
Tue, Apr 1, 8:28 AM
Unknown Object (File)
Tue, Apr 1, 8:28 AM
Unknown Object (File)
Tue, Apr 1, 8:28 AM
Unknown Object (File)
Tue, Apr 1, 8:28 AM
Unknown Object (File)
Tue, Apr 1, 4:56 AM
View All Files
Subscribers
ashoat

Details

Reviewers
atul
ginsu
tomek
Commits
rCOMMb1858ff3fc16: [web] Pop up a pin/unpin modal when the tooltip icon is clicked
Summary

Following the designs, we need an intermediate modal when a user tries to pin / unpin a message that 1) displays the message and 2) allows the user to confirm their requested action.

Because we are rendering a Message, there were two Providers we needed to avoid the invariants.

Firstly, the useMessageTooltipReplyAction requires an inputState. I define the inputState in tooltip-utils since the target message should
have access to the context, as opposed to it being null in the modal.

Secondly, the messageListContext is required to be defined since Message renders a TextMessage.

Linear: https://linear.app/comm/issue/ENG-3446/pop-up-a-pinunpin-modal-when-the-tooltip-icon-is-clicked

Depends on D7308

Test Plan

The video is linked below, I sync'd offline with Ted to confirm the designs

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

rohan created this revision.Apr 4 2023, 7:20 PM
Herald added a subscriber: ashoat. · View Herald TranscriptApr 4 2023, 7:20 PM
rohan edited the summary of this revision. (Show Details)Apr 4 2023, 7:21 PM
rohan edited the test plan for this revision. (Show Details)
rohan attached a referenced file: F463410: Screen Recording 2023-04-04 at 10.06.15 PM.mov. (Show Details)
rohan edited the summary of this revision. (Show Details)
rohan added inline comments.Apr 4 2023, 7:29 PM
web/modals/chat/toggle-pin-modal.react.js
59–64 ↗(On Diff #24649)

As explained in the inline comment, we want viewer to be false so the message is left-aligned and the username is displayed (feedback from Ted design wise)

65–70 ↗(On Diff #24649)

This is because we don't want to 'double render' the username and date for messages that will already have them rendered based on the conditions in the ComposedMessage component and the Message component

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 4 2023, 7:34 PM
Harbormaster failed remote builds in B17993: Diff 24649!
rohan added a child revision: D7310: [web] Display the number of pinned messages in a banner across chat.Apr 4 2023, 7:35 PM
rohan mentioned this in D7308: [lib/web] Add pin/unpin icon in the tooltip.Apr 5 2023, 5:27 AM
rohan updated this revision to Diff 24654.Apr 5 2023, 5:33 AM

Fix eslint warnings

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 5 2023, 5:50 AM
Harbormaster failed remote builds in B17996: Diff 24654!
rohan updated this revision to Diff 24958.Apr 11 2023, 5:05 AM

Update designs

rohan edited the summary of this revision. (Show Details)Apr 11 2023, 5:07 AM
rohan added inline comments.Apr 11 2023, 5:09 AM
web/components/pinned-message.react.js
31–34 ↗(On Diff #24958)

This is because we don't want to 'double render' the username, since we're rendering Message, which renders ComposedMessage, the component sometimes displays the username inherently based on some conditions.

web/modals/chat/toggle-pin-modal.react.js
94 ↗(On Diff #24958)

I separated this into it's own component because we need to re-use PinnedMessage when the user wants to view a list of all pinned messages for the thread in a separate modal.

rohan edited the test plan for this revision. (Show Details)Apr 11 2023, 5:11 AM
rohan attached a referenced file: F478765: Screen Recording 2023-04-11 at 8.09.58 AM.mov. (Show Details)
Harbormaster returned this revision to the author for changes because remote builds failed.Apr 11 2023, 5:21 AM
Harbormaster failed remote builds in B18222: Diff 24958!
rohan requested review of this revision.Apr 11 2023, 5:29 AM

I still need to figure out what is causing tooltip-utils.test.js to fail, but after attempting to debug locally, I'd prefer to unblock review and continuing figuring it out locally

rohan mentioned this in D7310: [web] Display the number of pinned messages in a banner across chat.Apr 11 2023, 5:30 AM
rohan mentioned this in D7380: [web] Display the pinned messages for the thread when the banner is clicked.Apr 11 2023, 6:02 AM
rohan mentioned this in D7382: [web] Allow interaction with inline engagement from the thread pinned messages modal.Apr 11 2023, 8:26 AM
tomek requested changes to this revision.Apr 12 2023, 4:46 AM

This diff causes eslint to fail. Please fix it.

web/components/pinned-message.css
6 ↗(On Diff #24958)

You can use a shorthand

20 ↗(On Diff #24958)

Do we have a color constant that can be used? If not, can we create one?

21 ↗(On Diff #24958)

We should use a const for it.

27 ↗(On Diff #24958)

Is there a way to avoid negative margin? Why it is needed?

web/modals/chat/toggle-pin-modal.react.js
36–43 ↗(On Diff #24958)

This might be a bit hard to maintain - the same condition is repeated a couple of times. I think it would be better if we had two objects with name, action, confirmationText, etc. and we choose one or another, based on a flag.

50–62 ↗(On Diff #24958)

It feels like we should have an intermediate layer which splits a description of message props from its appearance. By this I mean that we could introduce an object with e.g. isLeftAligned, hasHeader, etc. For an ordinary message we would then have { isLeftAligned: !isViewer, hasHeader: startsConversation, ...}, and for this message { isLeftAligned: true, hasHeader: true, ...}. This refactoring might touch a couple of places, so we can postpone it, but overall I think it will be beneficial.

69–81 ↗(On Diff #24958)

Immediately invoked functions reduce readability... in this case we should be able to easily avoid it.

82 ↗(On Diff #24958)

So we're closing the popup before the response from the server, right?

web/utils/tooltip-utils.js
510–514 ↗(On Diff #24958)

I think this might go really wrong. By introducing a new context we're splitting the data into two source of truth - which means that any modification to one won't be reflected in the other. Can we avoid this? Is the whole state needed for a message? Can we move the context above the modal?

This revision now requires changes to proceed.Apr 12 2023, 4:46 AM
rohan added a comment.Apr 12 2023, 11:36 PM

All other comments are addressed in the revision

web/modals/chat/toggle-pin-modal.react.js
50–62 ↗(On Diff #24958)

Makes sense, I will take a brief look at whether it makes sense to do it now or postpone it like you suggest, and after make a Linear task to not lose track of it

82 ↗(On Diff #24958)

Yeah, I thought this would be safe since we optimistically create the message as is, but I can add a follow-up / update this diff to change the behavior accordingly if needed

web/utils/tooltip-utils.js
510–514 ↗(On Diff #24958)

From what I experienced, since we're essentially rendering the Message component within the modal, without the context, the invariant here gets triggered.

In terms of moving the context above the modal, I couldn't find a good place to otherwise put it when going through the component hierarchy in dev tools (the Fragment representing the modal). I'm definitely open to suggestions though if you feel like it can be changed like you suggest

Screenshot 2023-04-13 at 7.34.38 AM.png (538×726 px, 87 KB)

rohan attached a referenced file: F482460: Screenshot 2023-04-13 at 7.34.38 AM.png. (Show Details)Apr 12 2023, 11:36 PM
rohan updated this revision to Diff 25073.EditedApr 12 2023, 11:40 PM

ESLint + Address Feedback

Checked the build logs, looked like ESLint warnings are now resolved, and the only cause for failure is the failing test that still needs to be addressed (this will propagate up the stack)

Screenshot 2023-04-13 at 7.47.14 AM.png (728×1 px, 188 KB)

rohan attached a referenced file: F482559: Screenshot 2023-04-13 at 7.47.14 AM.png. (Show Details)Apr 12 2023, 11:48 PM
Harbormaster failed remote builds in B18295: Diff 25073!Apr 12 2023, 11:56 PM
tomek accepted this revision.Apr 13 2023, 3:18 AM
tomek added inline comments.
web/components/pinned-message.react.js
31–34 ↗(On Diff #25073)

Are we sure about displaying this conditionally? Don't we want to always display the username for a pinned message?

web/modals/chat/toggle-pin-modal.css
3 ↗(On Diff #25073)

We can use a shorthand here, but 3-arg shorthands seem to be confusing for people, and we probably should keep it as is.

8 ↗(On Diff #25073)

Are we sure about this? Shouldn't we have a margin instead?

web/modals/chat/toggle-pin-modal.react.js
63–79 ↗(On Diff #25073)

This can be slightly simplified by using early exit

82 ↗(On Diff #24958)

Optimistic creation makes sense

This revision is now accepted and ready to land.Apr 13 2023, 3:18 AM
rohan added inline comments.Apr 17 2023, 5:00 AM
web/components/pinned-message.react.js
31–34 ↗(On Diff #25073)

Yeah, we always want to display the username for a pinned message. This condition is basically so we don't "double render" the username since ComposedMessage already will render a username in chat under a certain condition: https://github.com/CommE2E/comm/blob/master/web/chat/composed-message.react.js#L208-L209

web/modals/chat/toggle-pin-modal.css
8 ↗(On Diff #25073)

Yeah I can change this to a margin instead, may make maintaining it easier as well

rohan updated this revision to Diff 25341.Apr 19 2023, 6:11 AM

Address feedback

Harbormaster failed remote builds in B18505: Diff 25341!Apr 19 2023, 6:28 AM
ashoat added a comment.Apr 19 2023, 7:17 AM

@rohan, can you provide some clarification as to why you've been iterating on this stack for so long without resolving the unit test issue? Is it particularly challenging?

rohan added a comment.EditedApr 19 2023, 7:31 AM
In D7309#223179, @ashoat wrote:

@rohan, can you provide some clarification as to why you've been iterating on this stack for so long without resolving the unit test issue? Is it particularly challenging?

I spent a little time trying to figure it out, but I'm not entirely sure what change is causing it to break the unit test yet. I decided to finish addressing feedback and getting the rest of the code up for web first, so reviewers can at least provide feedback early on so all that is left to handle is just the unit test.

I'm still not 100% on what's causing it to fail, but I'm trying to figure it out locally next

Edit: This might be a circular dependency issue when I took a look using madge. Not sure if this is the cause yet

web/chat/message.react.js > web/chat/multimedia-message.react.js > web/chat/composed-message.react.js > web/utils/tooltip-utils.js > web/modals/chat/toggle-pin-modal.react.js > web/components/pinned-message.react.js
rohan updated this revision to Diff 25515.Apr 21 2023, 4:31 AM

Refactor parts of tooltip-utils.js out into tooltip-action-utils.js to resolve circular dependencies (should fix the failing unit test)

rohan updated this revision to Diff 25517.Apr 21 2023, 4:33 AM

Undo the commit, I should do it in the diff below this

rohan updated this revision to Diff 25520.Apr 21 2023, 4:39 AM

Rebase (expecting to see the unit test pass now) after running yarn workspace web test locally successfully

Harbormaster completed remote builds in B18625: Diff 25515.Apr 21 2023, 4:48 AM
Harbormaster failed remote builds in B18627: Diff 25517!
Harbormaster completed remote builds in B18630: Diff 25520.Apr 21 2023, 4:56 AM
rohan updated this revision to Diff 25534.Apr 21 2023, 7:41 AM

Rename PinnedMessage -> MessageResult

Harbormaster completed remote builds in B18643: Diff 25534.Apr 21 2023, 7:58 AM
rohan mentioned this in D7607: [native] Create the toggle pin modal.Apr 25 2023, 5:56 AM
rohan updated this revision to Diff 25729.Apr 25 2023, 5:32 PM

Rebase

Harbormaster completed remote builds in B18780: Diff 25729.Apr 25 2023, 5:49 PM
Closed by commit rCOMMb1858ff3fc16: [web] Pop up a pin/unpin modal when the tooltip icon is clicked (authored by RohanK6 <rohan.khanderia@gmail.com>, committed by rohan). · Explain WhyApr 25 2023, 6:00 PM
This revision was automatically updated to reflect the committed changes.
rohan added a commit: rCOMMb1858ff3fc16: [web] Pop up a pin/unpin modal when the tooltip icon is clicked.
rohan mentioned this in rCOMM12ded3a148fd: [native] Create the toggle pin modal.May 8 2023, 12:38 PM

Revision Contents
Changeset List

PathSize
web/
components/
28 lines
57 lines
modals/
chat/
30 lines
115 lines
2 lines
utils/
10 lines

Diff 25517

View Options

web/components/pinned-message.css

#pragma once #pragma once
#include "BlobGetClientReactor.h" #include "BlobGetClientReactor.h"
#include "DatabaseEntitiesTools.h"
#include "DatabaseManager.h" #include "DatabaseManager.h"
#include "ServerWriteReactorBase.h" #include "ServerWriteReactorBase.h"
#include "ServiceBlobClient.h" #include "ServiceBlobClient.h"
#include "../_generated/backup.grpc.pb.h" #include "../_generated/backup.grpc.pb.h"
#include "../_generated/backup.pb.h" #include "../_generated/backup.pb.h"
#include <folly/MPMCQueue.h> #include <folly/MPMCQueue.h>
#include <iostream> #include <iostream>
#include <memory> #include <memory>
#include <string> #include <string>
#include <vector>
namespace comm { namespace comm {
namespace network { namespace network {
namespace reactor { namespace reactor {
class PullBackupReactor : public ServerWriteReactorBase< class PullBackupReactor : public ServerWriteReactorBase<
backup::PullBackupRequest, backup::PullBackupRequest,
backup::PullBackupResponse> { backup::PullBackupResponse> {
enum class State { enum class State {
COMPACTION = 1, COMPACTION = 1,
LOGS = 2, LOGS = 2,
}; };
std::shared_ptr<database::BackupItem> backupItem; std::shared_ptr<database::BackupItem> backupItem;
std::shared_ptr<reactor::BlobGetClientReactor> getReactor; std::shared_ptr<reactor::BlobGetClientReactor> getReactor;
std::mutex reactorStateMutex; std::mutex reactorStateMutex;
std::shared_ptr<folly::MPMCQueue<std::string>> dataChunks; std::shared_ptr<folly::MPMCQueue<std::string>> dataChunks;
ServiceBlobClient blobClient; ServiceBlobClient blobClient;
State state = State::COMPACTION; State state = State::COMPACTION;
std::vector<std::shared_ptr<database::LogItem>> logs;
size_t currentLogIndex = 0;
std::shared_ptr<database::LogItem> currentLog;
void initializeGetReactor(const std::string &holder); void initializeGetReactor(const std::string &holder);
public: public:
PullBackupReactor(const backup::PullBackupRequest *request); PullBackupReactor(const backup::PullBackupRequest *request);
void initialize() override; void initialize() override;
Show All 34 Linesvoid PullBackupReactor::initialize() {
this->backupItem = database::DatabaseManager::getInstance().findBackupItem( this->backupItem = database::DatabaseManager::getInstance().findBackupItem(
this->request.userid(), this->request.backupid()); this->request.userid(), this->request.backupid());
if (this->backupItem == nullptr) { if (this->backupItem == nullptr) {
throw std::runtime_error( throw std::runtime_error(
"no backup found for provided parameters: user id [" + "no backup found for provided parameters: user id [" +
this->request.userid() + "], backup id [" + this->request.backupid() + this->request.userid() + "], backup id [" + this->request.backupid() +
"]"); "]");
} }
// TODO get logs this->logs = database::DatabaseManager::getInstance().findLogItemsForBackup(
this->request.backupid());
tomekUnsubmitted
Not Done
Inline Actions

I remember asking about it somewhere and you've probably answered, but what is the plan for a scenario where someone appends a log after we started reading but before we finish?

tomek: I remember asking about it somewhere and you've probably answered, but what is the plan for a…
karolAuthorUnsubmitted
Done
Inline Actions

I honestly don't remember a discussion like this.

What plan could be there? We're going to read the current state of the logs. It feels a bit weird to see a scenario containing two people. Backups are individual, right?

karol: I honestly don't remember a discussion like this. What plan could be there? We're going to…
tomekUnsubmitted
Not Done
Inline Actions

Ok, so let's be more precise. By someone, I meant two different clients, and in that case the same user could be logged in on both.

There are three ways of handling this:

  1. We can raise an error - we really don't want that
  2. We can deliver a backup with a state from the begging of the operation. This is what you suggested.
  3. We can deliver a backup with a state from the end of the operation.

I think that option 2 is easier, but at the same time we provide something which we know is outdated. Option 3 can be implemented e.g. by checking if there are new entries when we finished sending logs.

Option 2 has a big issue when we restore the state from a backup - we might miss some updates that were made after we started the process. We can work-around it by e.g. having currentAsOf field and fetch new logs from the client after the backup is restored. So overall, both options are ok, but we should choose one and have it in our minds while implementing the rest of the system.

@karol-bisztyga @ashoat

tomek: Ok, so let's be more precise. By someone, I meant two different clients, and in that case the…
ashoatUnsubmitted
Not Done
Inline Actions

The diff was landed without replying to this question!

ashoat: The diff was landed without replying to this question!
} }
std::unique_ptr<grpc::Status> std::unique_ptr<grpc::Status>
PullBackupReactor::writeResponse(backup::PullBackupResponse *response) { PullBackupReactor::writeResponse(backup::PullBackupResponse *response) {
// we make sure that the blob client's state is flushed to the main memory // we make sure that the blob client's state is flushed to the main memory
// as there may be multiple threads from the pool taking over here // as there may be multiple threads from the pool taking over here
const std::lock_guard<std::mutex> lock(this->reactorStateMutex); const std::lock_guard<std::mutex> lock(this->reactorStateMutex);
switch (this->state) { if (this->state == State::COMPACTION) {
case State::COMPACTION: {
this->initializeGetReactor(this->backupItem->getCompactionHolder()); this->initializeGetReactor(this->backupItem->getCompactionHolder());
std::string dataChunk; std::string dataChunk;
this->dataChunks->blockingRead(dataChunk); this->dataChunks->blockingRead(dataChunk);
if (dataChunk.empty()) { if (!dataChunk.empty()) {
// TODO try to immediately start writing logs instead of wasting a cycle response->set_compactionchunk(dataChunk);
// sending nothing
this->state = State::LOGS;
return nullptr; return nullptr;
} }
response->set_compactionchunk(dataChunk); if (!this->dataChunks->isEmpty()) {
throw std::runtime_error(
"dangling data discovered after reading compaction");
}
this->getReactor = nullptr;
this->state = State::LOGS;
}
if (this->state == State::LOGS) {
// TODO make sure logs are received in correct order regardless their size
if (this->logs.empty()) {
return std::make_unique<grpc::Status>(grpc::Status::OK);
}
if (this->currentLogIndex == this->logs.size()) {
return std::make_unique<grpc::Status>(grpc::Status::OK);
}
if (this->currentLogIndex > this->logs.size()) {
throw std::runtime_error("log index out of bound");
}
if (this->currentLog == nullptr) {
this->currentLog = this->logs.at(this->currentLogIndex);
if (this->currentLog->getPersistedInBlob()) {
this->initializeGetReactor(this->currentLog->getValue());
} else {
response->set_logchunk(this->currentLog->getValue());
++this->currentLogIndex;
this->currentLog = nullptr;
return nullptr; return nullptr;
} }
case State::LOGS: {
throw std::runtime_error("unimplemented");
} }
std::string dataChunk;
this->dataChunks->blockingRead(dataChunk);
if (dataChunk.empty()) {
++this->currentLogIndex;
this->currentLog = nullptr;
} else {
response->set_logchunk(dataChunk);
}
return nullptr;
} }
throw std::runtime_error("unhandled state"); throw std::runtime_error("unhandled state");
} }
} // namespace reactor } // namespace reactor
} // namespace network } // namespace network
} // namespace comm } // namespace comm
View Options

web/components/pinned-message.react.js

#pragma once #pragma once
#include "BlobGetClientReactor.h" #include "BlobGetClientReactor.h"
#include "DatabaseEntitiesTools.h"
#include "DatabaseManager.h" #include "DatabaseManager.h"
#include "ServerWriteReactorBase.h" #include "ServerWriteReactorBase.h"
#include "ServiceBlobClient.h" #include "ServiceBlobClient.h"
#include "../_generated/backup.grpc.pb.h" #include "../_generated/backup.grpc.pb.h"
#include "../_generated/backup.pb.h" #include "../_generated/backup.pb.h"
#include <folly/MPMCQueue.h> #include <folly/MPMCQueue.h>
#include <iostream> #include <iostream>
#include <memory> #include <memory>
#include <string> #include <string>
#include <vector>
namespace comm { namespace comm {
namespace network { namespace network {
namespace reactor { namespace reactor {
class PullBackupReactor : public ServerWriteReactorBase< class PullBackupReactor : public ServerWriteReactorBase<
backup::PullBackupRequest, backup::PullBackupRequest,
backup::PullBackupResponse> { backup::PullBackupResponse> {
enum class State { enum class State {
COMPACTION = 1, COMPACTION = 1,
LOGS = 2, LOGS = 2,
}; };
std::shared_ptr<database::BackupItem> backupItem; std::shared_ptr<database::BackupItem> backupItem;
std::shared_ptr<reactor::BlobGetClientReactor> getReactor; std::shared_ptr<reactor::BlobGetClientReactor> getReactor;
std::mutex reactorStateMutex; std::mutex reactorStateMutex;
std::shared_ptr<folly::MPMCQueue<std::string>> dataChunks; std::shared_ptr<folly::MPMCQueue<std::string>> dataChunks;
ServiceBlobClient blobClient; ServiceBlobClient blobClient;
State state = State::COMPACTION; State state = State::COMPACTION;
std::vector<std::shared_ptr<database::LogItem>> logs;
size_t currentLogIndex = 0;
std::shared_ptr<database::LogItem> currentLog;
void initializeGetReactor(const std::string &holder); void initializeGetReactor(const std::string &holder);
public: public:
PullBackupReactor(const backup::PullBackupRequest *request); PullBackupReactor(const backup::PullBackupRequest *request);
void initialize() override; void initialize() override;
Show All 34 Linesvoid PullBackupReactor::initialize() {
this->backupItem = database::DatabaseManager::getInstance().findBackupItem( this->backupItem = database::DatabaseManager::getInstance().findBackupItem(
this->request.userid(), this->request.backupid()); this->request.userid(), this->request.backupid());
if (this->backupItem == nullptr) { if (this->backupItem == nullptr) {
throw std::runtime_error( throw std::runtime_error(
"no backup found for provided parameters: user id [" + "no backup found for provided parameters: user id [" +
this->request.userid() + "], backup id [" + this->request.backupid() + this->request.userid() + "], backup id [" + this->request.backupid() +
"]"); "]");
} }
// TODO get logs this->logs = database::DatabaseManager::getInstance().findLogItemsForBackup(
this->request.backupid());
tomekUnsubmitted
Not Done
Inline Actions

I remember asking about it somewhere and you've probably answered, but what is the plan for a scenario where someone appends a log after we started reading but before we finish?

tomek: I remember asking about it somewhere and you've probably answered, but what is the plan for a…
karolAuthorUnsubmitted
Done
Inline Actions

I honestly don't remember a discussion like this.

What plan could be there? We're going to read the current state of the logs. It feels a bit weird to see a scenario containing two people. Backups are individual, right?

karol: I honestly don't remember a discussion like this. What plan could be there? We're going to…
tomekUnsubmitted
Not Done
Inline Actions

Ok, so let's be more precise. By someone, I meant two different clients, and in that case the same user could be logged in on both.

There are three ways of handling this:

  1. We can raise an error - we really don't want that
  2. We can deliver a backup with a state from the begging of the operation. This is what you suggested.
  3. We can deliver a backup with a state from the end of the operation.

I think that option 2 is easier, but at the same time we provide something which we know is outdated. Option 3 can be implemented e.g. by checking if there are new entries when we finished sending logs.

Option 2 has a big issue when we restore the state from a backup - we might miss some updates that were made after we started the process. We can work-around it by e.g. having currentAsOf field and fetch new logs from the client after the backup is restored. So overall, both options are ok, but we should choose one and have it in our minds while implementing the rest of the system.

@karol-bisztyga @ashoat

tomek: Ok, so let's be more precise. By someone, I meant two different clients, and in that case the…
ashoatUnsubmitted
Not Done
Inline Actions

The diff was landed without replying to this question!

ashoat: The diff was landed without replying to this question!
} }
std::unique_ptr<grpc::Status> std::unique_ptr<grpc::Status>
PullBackupReactor::writeResponse(backup::PullBackupResponse *response) { PullBackupReactor::writeResponse(backup::PullBackupResponse *response) {
// we make sure that the blob client's state is flushed to the main memory // we make sure that the blob client's state is flushed to the main memory
// as there may be multiple threads from the pool taking over here // as there may be multiple threads from the pool taking over here
const std::lock_guard<std::mutex> lock(this->reactorStateMutex); const std::lock_guard<std::mutex> lock(this->reactorStateMutex);
switch (this->state) { if (this->state == State::COMPACTION) {
case State::COMPACTION: {
this->initializeGetReactor(this->backupItem->getCompactionHolder()); this->initializeGetReactor(this->backupItem->getCompactionHolder());
std::string dataChunk; std::string dataChunk;
this->dataChunks->blockingRead(dataChunk); this->dataChunks->blockingRead(dataChunk);
if (dataChunk.empty()) { if (!dataChunk.empty()) {
// TODO try to immediately start writing logs instead of wasting a cycle response->set_compactionchunk(dataChunk);
// sending nothing
this->state = State::LOGS;
return nullptr; return nullptr;
} }
response->set_compactionchunk(dataChunk); if (!this->dataChunks->isEmpty()) {
throw std::runtime_error(
"dangling data discovered after reading compaction");
}
this->getReactor = nullptr;
this->state = State::LOGS;
}
if (this->state == State::LOGS) {
// TODO make sure logs are received in correct order regardless their size
if (this->logs.empty()) {
return std::make_unique<grpc::Status>(grpc::Status::OK);
}
if (this->currentLogIndex == this->logs.size()) {
return std::make_unique<grpc::Status>(grpc::Status::OK);
}
if (this->currentLogIndex > this->logs.size()) {
throw std::runtime_error("log index out of bound");
}
if (this->currentLog == nullptr) {
this->currentLog = this->logs.at(this->currentLogIndex);
if (this->currentLog->getPersistedInBlob()) {
this->initializeGetReactor(this->currentLog->getValue());
} else {
response->set_logchunk(this->currentLog->getValue());
++this->currentLogIndex;
this->currentLog = nullptr;
return nullptr; return nullptr;
} }
case State::LOGS: {
throw std::runtime_error("unimplemented");
} }
std::string dataChunk;
this->dataChunks->blockingRead(dataChunk);
if (dataChunk.empty()) {
++this->currentLogIndex;
this->currentLog = nullptr;
} else {
response->set_logchunk(dataChunk);
}
return nullptr;
} }
throw std::runtime_error("unhandled state"); throw std::runtime_error("unhandled state");
} }
} // namespace reactor } // namespace reactor
} // namespace network } // namespace network
} // namespace comm } // namespace comm
View Options

web/modals/chat/toggle-pin-modal.css

#pragma once #pragma once
#include "BlobGetClientReactor.h" #include "BlobGetClientReactor.h"
#include "DatabaseEntitiesTools.h"
#include "DatabaseManager.h" #include "DatabaseManager.h"
#include "ServerWriteReactorBase.h" #include "ServerWriteReactorBase.h"
#include "ServiceBlobClient.h" #include "ServiceBlobClient.h"
#include "../_generated/backup.grpc.pb.h" #include "../_generated/backup.grpc.pb.h"
#include "../_generated/backup.pb.h" #include "../_generated/backup.pb.h"
#include <folly/MPMCQueue.h> #include <folly/MPMCQueue.h>
#include <iostream> #include <iostream>
#include <memory> #include <memory>
#include <string> #include <string>
#include <vector>
namespace comm { namespace comm {
namespace network { namespace network {
namespace reactor { namespace reactor {
class PullBackupReactor : public ServerWriteReactorBase< class PullBackupReactor : public ServerWriteReactorBase<
backup::PullBackupRequest, backup::PullBackupRequest,
backup::PullBackupResponse> { backup::PullBackupResponse> {
enum class State { enum class State {
COMPACTION = 1, COMPACTION = 1,
LOGS = 2, LOGS = 2,
}; };
std::shared_ptr<database::BackupItem> backupItem; std::shared_ptr<database::BackupItem> backupItem;
std::shared_ptr<reactor::BlobGetClientReactor> getReactor; std::shared_ptr<reactor::BlobGetClientReactor> getReactor;
std::mutex reactorStateMutex; std::mutex reactorStateMutex;
std::shared_ptr<folly::MPMCQueue<std::string>> dataChunks; std::shared_ptr<folly::MPMCQueue<std::string>> dataChunks;
ServiceBlobClient blobClient; ServiceBlobClient blobClient;
State state = State::COMPACTION; State state = State::COMPACTION;
std::vector<std::shared_ptr<database::LogItem>> logs;
size_t currentLogIndex = 0;
std::shared_ptr<database::LogItem> currentLog;
void initializeGetReactor(const std::string &holder); void initializeGetReactor(const std::string &holder);
public: public:
PullBackupReactor(const backup::PullBackupRequest *request); PullBackupReactor(const backup::PullBackupRequest *request);
void initialize() override; void initialize() override;
Show All 34 Linesvoid PullBackupReactor::initialize() {
this->backupItem = database::DatabaseManager::getInstance().findBackupItem( this->backupItem = database::DatabaseManager::getInstance().findBackupItem(
this->request.userid(), this->request.backupid()); this->request.userid(), this->request.backupid());
if (this->backupItem == nullptr) { if (this->backupItem == nullptr) {
throw std::runtime_error( throw std::runtime_error(
"no backup found for provided parameters: user id [" + "no backup found for provided parameters: user id [" +
this->request.userid() + "], backup id [" + this->request.backupid() + this->request.userid() + "], backup id [" + this->request.backupid() +
"]"); "]");
} }
// TODO get logs this->logs = database::DatabaseManager::getInstance().findLogItemsForBackup(
this->request.backupid());
tomekUnsubmitted
Not Done
Inline Actions

I remember asking about it somewhere and you've probably answered, but what is the plan for a scenario where someone appends a log after we started reading but before we finish?

tomek: I remember asking about it somewhere and you've probably answered, but what is the plan for a…
karolAuthorUnsubmitted
Done
Inline Actions

I honestly don't remember a discussion like this.

What plan could be there? We're going to read the current state of the logs. It feels a bit weird to see a scenario containing two people. Backups are individual, right?

karol: I honestly don't remember a discussion like this. What plan could be there? We're going to…
tomekUnsubmitted
Not Done
Inline Actions

Ok, so let's be more precise. By someone, I meant two different clients, and in that case the same user could be logged in on both.

There are three ways of handling this:

  1. We can raise an error - we really don't want that
  2. We can deliver a backup with a state from the begging of the operation. This is what you suggested.
  3. We can deliver a backup with a state from the end of the operation.

I think that option 2 is easier, but at the same time we provide something which we know is outdated. Option 3 can be implemented e.g. by checking if there are new entries when we finished sending logs.

Option 2 has a big issue when we restore the state from a backup - we might miss some updates that were made after we started the process. We can work-around it by e.g. having currentAsOf field and fetch new logs from the client after the backup is restored. So overall, both options are ok, but we should choose one and have it in our minds while implementing the rest of the system.

@karol-bisztyga @ashoat

tomek: Ok, so let's be more precise. By someone, I meant two different clients, and in that case the…
ashoatUnsubmitted
Not Done
Inline Actions

The diff was landed without replying to this question!

ashoat: The diff was landed without replying to this question!
} }
std::unique_ptr<grpc::Status> std::unique_ptr<grpc::Status>
PullBackupReactor::writeResponse(backup::PullBackupResponse *response) { PullBackupReactor::writeResponse(backup::PullBackupResponse *response) {
// we make sure that the blob client's state is flushed to the main memory // we make sure that the blob client's state is flushed to the main memory
// as there may be multiple threads from the pool taking over here // as there may be multiple threads from the pool taking over here
const std::lock_guard<std::mutex> lock(this->reactorStateMutex); const std::lock_guard<std::mutex> lock(this->reactorStateMutex);
switch (this->state) { if (this->state == State::COMPACTION) {
case State::COMPACTION: {
this->initializeGetReactor(this->backupItem->getCompactionHolder()); this->initializeGetReactor(this->backupItem->getCompactionHolder());
std::string dataChunk; std::string dataChunk;
this->dataChunks->blockingRead(dataChunk); this->dataChunks->blockingRead(dataChunk);
if (dataChunk.empty()) { if (!dataChunk.empty()) {
// TODO try to immediately start writing logs instead of wasting a cycle response->set_compactionchunk(dataChunk);
// sending nothing
this->state = State::LOGS;
return nullptr; return nullptr;
} }
response->set_compactionchunk(dataChunk); if (!this->dataChunks->isEmpty()) {
throw std::runtime_error(
"dangling data discovered after reading compaction");
}
this->getReactor = nullptr;
this->state = State::LOGS;
}
if (this->state == State::LOGS) {
// TODO make sure logs are received in correct order regardless their size
if (this->logs.empty()) {
return std::make_unique<grpc::Status>(grpc::Status::OK);
}
if (this->currentLogIndex == this->logs.size()) {
return std::make_unique<grpc::Status>(grpc::Status::OK);
}
if (this->currentLogIndex > this->logs.size()) {
throw std::runtime_error("log index out of bound");
}
if (this->currentLog == nullptr) {
this->currentLog = this->logs.at(this->currentLogIndex);
if (this->currentLog->getPersistedInBlob()) {
this->initializeGetReactor(this->currentLog->getValue());
} else {
response->set_logchunk(this->currentLog->getValue());
++this->currentLogIndex;
this->currentLog = nullptr;
return nullptr; return nullptr;
} }
case State::LOGS: {
throw std::runtime_error("unimplemented");
} }
std::string dataChunk;
this->dataChunks->blockingRead(dataChunk);
if (dataChunk.empty()) {
++this->currentLogIndex;
this->currentLog = nullptr;
} else {
response->set_logchunk(dataChunk);
}
return nullptr;
} }
throw std::runtime_error("unhandled state"); throw std::runtime_error("unhandled state");
} }
} // namespace reactor } // namespace reactor
} // namespace network } // namespace network
} // namespace comm } // namespace comm
View Options

web/modals/chat/toggle-pin-modal.react.js

#pragma once #pragma once
#include "BlobGetClientReactor.h" #include "BlobGetClientReactor.h"
#include "DatabaseEntitiesTools.h"
#include "DatabaseManager.h" #include "DatabaseManager.h"
#include "ServerWriteReactorBase.h" #include "ServerWriteReactorBase.h"
#include "ServiceBlobClient.h" #include "ServiceBlobClient.h"
#include "../_generated/backup.grpc.pb.h" #include "../_generated/backup.grpc.pb.h"
#include "../_generated/backup.pb.h" #include "../_generated/backup.pb.h"
#include <folly/MPMCQueue.h> #include <folly/MPMCQueue.h>
#include <iostream> #include <iostream>
#include <memory> #include <memory>
#include <string> #include <string>
#include <vector>
namespace comm { namespace comm {
namespace network { namespace network {
namespace reactor { namespace reactor {
class PullBackupReactor : public ServerWriteReactorBase< class PullBackupReactor : public ServerWriteReactorBase<
backup::PullBackupRequest, backup::PullBackupRequest,
backup::PullBackupResponse> { backup::PullBackupResponse> {
enum class State { enum class State {
COMPACTION = 1, COMPACTION = 1,
LOGS = 2, LOGS = 2,
}; };
std::shared_ptr<database::BackupItem> backupItem; std::shared_ptr<database::BackupItem> backupItem;
std::shared_ptr<reactor::BlobGetClientReactor> getReactor; std::shared_ptr<reactor::BlobGetClientReactor> getReactor;
std::mutex reactorStateMutex; std::mutex reactorStateMutex;
std::shared_ptr<folly::MPMCQueue<std::string>> dataChunks; std::shared_ptr<folly::MPMCQueue<std::string>> dataChunks;
ServiceBlobClient blobClient; ServiceBlobClient blobClient;
State state = State::COMPACTION; State state = State::COMPACTION;
std::vector<std::shared_ptr<database::LogItem>> logs;
size_t currentLogIndex = 0;
std::shared_ptr<database::LogItem> currentLog;
void initializeGetReactor(const std::string &holder); void initializeGetReactor(const std::string &holder);
public: public:
PullBackupReactor(const backup::PullBackupRequest *request); PullBackupReactor(const backup::PullBackupRequest *request);
void initialize() override; void initialize() override;
Show All 34 Linesvoid PullBackupReactor::initialize() {
this->backupItem = database::DatabaseManager::getInstance().findBackupItem( this->backupItem = database::DatabaseManager::getInstance().findBackupItem(
this->request.userid(), this->request.backupid()); this->request.userid(), this->request.backupid());
if (this->backupItem == nullptr) { if (this->backupItem == nullptr) {
throw std::runtime_error( throw std::runtime_error(
"no backup found for provided parameters: user id [" + "no backup found for provided parameters: user id [" +
this->request.userid() + "], backup id [" + this->request.backupid() + this->request.userid() + "], backup id [" + this->request.backupid() +
"]"); "]");
} }
// TODO get logs this->logs = database::DatabaseManager::getInstance().findLogItemsForBackup(
this->request.backupid());
tomekUnsubmitted
Not Done
Inline Actions

I remember asking about it somewhere and you've probably answered, but what is the plan for a scenario where someone appends a log after we started reading but before we finish?

tomek: I remember asking about it somewhere and you've probably answered, but what is the plan for a…
karolAuthorUnsubmitted
Done
Inline Actions

I honestly don't remember a discussion like this.

What plan could be there? We're going to read the current state of the logs. It feels a bit weird to see a scenario containing two people. Backups are individual, right?

karol: I honestly don't remember a discussion like this. What plan could be there? We're going to…
tomekUnsubmitted
Not Done
Inline Actions

Ok, so let's be more precise. By someone, I meant two different clients, and in that case the same user could be logged in on both.

There are three ways of handling this:

  1. We can raise an error - we really don't want that
  2. We can deliver a backup with a state from the begging of the operation. This is what you suggested.
  3. We can deliver a backup with a state from the end of the operation.

I think that option 2 is easier, but at the same time we provide something which we know is outdated. Option 3 can be implemented e.g. by checking if there are new entries when we finished sending logs.

Option 2 has a big issue when we restore the state from a backup - we might miss some updates that were made after we started the process. We can work-around it by e.g. having currentAsOf field and fetch new logs from the client after the backup is restored. So overall, both options are ok, but we should choose one and have it in our minds while implementing the rest of the system.

@karol-bisztyga @ashoat

tomek: Ok, so let's be more precise. By someone, I meant two different clients, and in that case the…
ashoatUnsubmitted
Not Done
Inline Actions

The diff was landed without replying to this question!

ashoat: The diff was landed without replying to this question!
} }
std::unique_ptr<grpc::Status> std::unique_ptr<grpc::Status>
PullBackupReactor::writeResponse(backup::PullBackupResponse *response) { PullBackupReactor::writeResponse(backup::PullBackupResponse *response) {
// we make sure that the blob client's state is flushed to the main memory // we make sure that the blob client's state is flushed to the main memory
// as there may be multiple threads from the pool taking over here // as there may be multiple threads from the pool taking over here
const std::lock_guard<std::mutex> lock(this->reactorStateMutex); const std::lock_guard<std::mutex> lock(this->reactorStateMutex);
switch (this->state) { if (this->state == State::COMPACTION) {
case State::COMPACTION: {
this->initializeGetReactor(this->backupItem->getCompactionHolder()); this->initializeGetReactor(this->backupItem->getCompactionHolder());
std::string dataChunk; std::string dataChunk;
this->dataChunks->blockingRead(dataChunk); this->dataChunks->blockingRead(dataChunk);
if (dataChunk.empty()) { if (!dataChunk.empty()) {
// TODO try to immediately start writing logs instead of wasting a cycle response->set_compactionchunk(dataChunk);
// sending nothing
this->state = State::LOGS;
return nullptr; return nullptr;
} }
response->set_compactionchunk(dataChunk); if (!this->dataChunks->isEmpty()) {
throw std::runtime_error(
"dangling data discovered after reading compaction");
}
this->getReactor = nullptr;
this->state = State::LOGS;
}
if (this->state == State::LOGS) {
// TODO make sure logs are received in correct order regardless their size
if (this->logs.empty()) {
return std::make_unique<grpc::Status>(grpc::Status::OK);
}
if (this->currentLogIndex == this->logs.size()) {
return std::make_unique<grpc::Status>(grpc::Status::OK);
}
if (this->currentLogIndex > this->logs.size()) {
throw std::runtime_error("log index out of bound");
}
if (this->currentLog == nullptr) {
this->currentLog = this->logs.at(this->currentLogIndex);
if (this->currentLog->getPersistedInBlob()) {
this->initializeGetReactor(this->currentLog->getValue());
} else {
response->set_logchunk(this->currentLog->getValue());
++this->currentLogIndex;
this->currentLog = nullptr;
return nullptr; return nullptr;
} }
case State::LOGS: {
throw std::runtime_error("unimplemented");
} }
std::string dataChunk;
this->dataChunks->blockingRead(dataChunk);
if (dataChunk.empty()) {
++this->currentLogIndex;
this->currentLog = nullptr;
} else {
response->set_logchunk(dataChunk);
}
return nullptr;
} }
throw std::runtime_error("unhandled state"); throw std::runtime_error("unhandled state");
} }
} // namespace reactor } // namespace reactor
} // namespace network } // namespace network
} // namespace comm } // namespace comm
View Options

web/theme.css

Loading...
View Options

web/utils/tooltip-utils.js

Loading...