[bisq-network/bisq] Reduce initial request size (#4233)
sqrrm
notifications at github.com
Tue May 19 17:38:50 UTC 2020
@sqrrm commented on this pull request.
I've read through the core parts of the PR. For now just adding comments to gain a better understanding. This PR makes me concerned that we might lose data during upgrades or otherwise, but I don't have a full understanding yet.
> + if (getMap().containsKey(hash))
+ return;
+
+ store.getMap().put(hash, payload);
+ persist();
+ }
+
+ @Override
+ protected PersistableNetworkPayload putIfAbsent(P2PDataStorage.ByteArray hash,
+ PersistableNetworkPayload payload) {
+ // make sure we do not add data that we already have (in a bin of historical data)
+ if (getMap().containsKey(hash))
+ return null;
+
+ PersistableNetworkPayload previous = store.getMap().putIfAbsent(hash, payload);
+ persist();
Seems excessive to persist if nothing was written, but I notice that's what MapStoreService does.
> + PersistableNetworkPayload payload) {
+ // make sure we do not add data that we already have (in a bin of historical data)
+ if (getMap().containsKey(hash))
+ return null;
+
+ PersistableNetworkPayload previous = store.getMap().putIfAbsent(hash, payload);
+ persist();
+ return previous;
+ }
+
+
+ @Override
+ public Map<P2PDataStorage.ByteArray, PersistableNetworkPayload> getMap() {
+ HashMap<P2PDataStorage.ByteArray, PersistableNetworkPayload> result = new HashMap<>();
+ result.putAll(store.getMap());
+ history.forEach((s, store) -> result.putAll(store.getMap()));
getMap() seems to be called a fair bit and it's recreating the map every time. Not sure how expensive that is, but perhaps some caching should be considered.
> + history.forEach((s, store) -> result.putAll(store.getMap()));
+
+ return result;
+ }
+
+ @Override
+ public Map<P2PDataStorage.ByteArray, PersistableNetworkPayload> getMap(String filter) {
+ HashMap<P2PDataStorage.ByteArray, PersistableNetworkPayload> result = new HashMap<>();
+ result.putAll(store.getMap());
+
+ // TODO do a proper language, possibly classes
+ if (filter.startsWith("since ")) {
+ filter = filter.replace("since ", "");
+ if (!filter.equals(Version.VERSION)) {
+ String finalFilter = filter;
+ history.entrySet().stream().filter(entry -> Integer.valueOf(entry.getKey().replace(".", "")) > Integer.valueOf(finalFilter.replace(".", ""))).forEach(entry -> result.putAll(entry.getValue().getMap()));
Try to keep lines < 120 chars
> + if (filter.startsWith("since ")) {
+ filter = filter.replace("since ", "");
+ if (!filter.equals(Version.VERSION)) {
+ String finalFilter = filter;
+ history.entrySet().stream().filter(entry -> Integer.valueOf(entry.getKey().replace(".", "")) > Integer.valueOf(finalFilter.replace(".", ""))).forEach(entry -> result.putAll(entry.getValue().getMap()));
+ }
+ }
+
+ return result;
+ }
+
+ @Override
+ protected void readFromResources(String postFix) {
+ // check Version.VERSION and see if we have latest r/o data store file in working directory
+ if (!new File(absolutePathOfStorageDir + File.separator + getFileName() + "_" + Version.VERSION).exists())
+ makeFileFromResourceFile(postFix); // if we have the latest file, we are good, else do stuff // TODO are we?
This checks if the latest file exists...
> + history.entrySet().stream().filter(entry -> Integer.valueOf(entry.getKey().replace(".", "")) > Integer.valueOf(finalFilter.replace(".", ""))).forEach(entry -> result.putAll(entry.getValue().getMap()));
+ }
+ }
+
+ return result;
+ }
+
+ @Override
+ protected void readFromResources(String postFix) {
+ // check Version.VERSION and see if we have latest r/o data store file in working directory
+ if (!new File(absolutePathOfStorageDir + File.separator + getFileName() + "_" + Version.VERSION).exists())
+ makeFileFromResourceFile(postFix); // if we have the latest file, we are good, else do stuff // TODO are we?
+ else {
+ // load stores/storage
+ File dbDir = new File(absolutePathOfStorageDir);
+ List<File> resourceFiles = Arrays.asList(dbDir.list((dir, name) -> name.startsWith(getFileName() + "_"))).stream().map(s -> new File(s)).collect(Collectors.toList());
... if it does exist we make a list of all the files we have.
Also, line breaking streams make them a lot easier to follow.
> +
+ @Override
+ protected void readFromResources(String postFix) {
+ // check Version.VERSION and see if we have latest r/o data store file in working directory
+ if (!new File(absolutePathOfStorageDir + File.separator + getFileName() + "_" + Version.VERSION).exists())
+ makeFileFromResourceFile(postFix); // if we have the latest file, we are good, else do stuff // TODO are we?
+ else {
+ // load stores/storage
+ File dbDir = new File(absolutePathOfStorageDir);
+ List<File> resourceFiles = Arrays.asList(dbDir.list((dir, name) -> name.startsWith(getFileName() + "_"))).stream().map(s -> new File(s)).collect(Collectors.toList());
+
+ history = new HashMap<>();
+ store = readStore(getFileName());
+ resourceFiles.forEach(file -> {
+ SplitStore tmp = readStore(file.getName().replace(postFix, ""));
+ history.put(file.getName().replace(postFix, "").replace(getFileName(), "").replace("_", ""), tmp);
Then we add them to our history. What is the key, not easy to follow. Do we depend on having all the previous stores since we're only checking for the last one?
> + }
+
+ @Override
+ protected void makeFileFromResourceFile(String postFix) {
+ File dbDir = new File(absolutePathOfStorageDir);
+ if (!dbDir.exists() && !dbDir.mkdir())
+ log.warn("make dir failed.\ndbDir=" + dbDir.getAbsolutePath());
+
+ // check resources for files
+ List<String> versions = new ArrayList<>();
+ versions.add(Version.VERSION);
+ versions.addAll(Version.history);
+ List<String> resourceFiles = versions.stream().map(s -> getFileName() + "_" + s + postFix).collect(Collectors.toList());
+
+ // if not, copy and split
+ resourceFiles.forEach(file -> {
`file` is a filename string...
> + @Override
+ protected void makeFileFromResourceFile(String postFix) {
+ File dbDir = new File(absolutePathOfStorageDir);
+ if (!dbDir.exists() && !dbDir.mkdir())
+ log.warn("make dir failed.\ndbDir=" + dbDir.getAbsolutePath());
+
+ // check resources for files
+ List<String> versions = new ArrayList<>();
+ versions.add(Version.VERSION);
+ versions.addAll(Version.history);
+ List<String> resourceFiles = versions.stream().map(s -> getFileName() + "_" + s + postFix).collect(Collectors.toList());
+
+ // if not, copy and split
+ resourceFiles.forEach(file -> {
+ final File destinationFile = new File(Paths.get(absolutePathOfStorageDir, file.replace(postFix, "")).toString());
+ final String resourceFileName = file;
...making this var a bit excessive.
> + log.error("Could not copy resourceFile " + resourceFileName + " to " +
+ destinationFile.getAbsolutePath() + ".\n" + e.getMessage());
+ e.printStackTrace();
+ }
+ } else {
+ log.debug(file + " file exists already.");
+ }
+ });
+
+ // split
+ // - get all
+ history = new HashMap<>();
+ store = readStore(getFileName());
+ resourceFiles.forEach(file -> {
+ SplitStore tmp = readStore(file.replace(postFix, ""));
+ history.put(file.replace(postFix, "").replace(getFileName(), "").replace("_", ""), tmp);
A helper method to do these string manipulations would help readability.
> + e.printStackTrace();
+ }
+ } else {
+ log.debug(file + " file exists already.");
+ }
+ });
+
+ // split
+ // - get all
+ history = new HashMap<>();
+ store = readStore(getFileName());
+ resourceFiles.forEach(file -> {
+ SplitStore tmp = readStore(file.replace(postFix, ""));
+ history.put(file.replace(postFix, "").replace(getFileName(), "").replace("_", ""), tmp);
+ // - subtract all that is in resource files
+ store.getMap().keySet().removeAll(tmp.getMap().keySet());
With this change I think we're making the resource files the final arbiter of what is the distributed data. Is that what we want? Even for full nodes that are always on.
>From what I understood so far, nodes will keep getting data from the network. With new versions all new data since the last version will be stored in a new `resource_x.x.x_MAINNET` file and all nodes will restore this data from resources. It looks like it will just throw away its own local data on new versions though, that seems incorrect.
> @@ -205,24 +213,47 @@ public GetUpdatedDataRequest buildGetUpdatedDataRequest(NodeAddress senderNodeAd
return new GetUpdatedDataRequest(senderNodeAddress, nonce, this.getKnownPayloadHashes());
}
+ private byte[] getSpecialKey() {
+ byte[] result = new byte[20];
+ Arrays.fill(result, (byte) 0);
+ System.arraycopy(Version.VERSION.getBytes(), 0, result, 0, Version.VERSION.length());
+ return result;
+ }
+
+ private String containsSpecialKey(Set<P2PDataStorage.ByteArray> collection) {
+ Optional<String> result = collection.stream().map(byteArray -> new String(byteArray.bytes).trim()).filter(s -> s.matches("^[0-9]\\.[0-9]\\.[0-9]$")).findFirst();
+ if (result.isPresent())
+ return result.get();
+ else
+ return "";
Easier to read would be ` return result.orElse("");`
or even
```
return collection.stream()
.map(byteArray -> new String(byteArray.bytes).trim())
.filter(s -> s.matches("^[0-9]\\.[0-9]\\.[0-9]$"))
.findFirst()
.orElse("");
```
> @@ -205,24 +213,47 @@ public GetUpdatedDataRequest buildGetUpdatedDataRequest(NodeAddress senderNodeAd
return new GetUpdatedDataRequest(senderNodeAddress, nonce, this.getKnownPayloadHashes());
}
+ private byte[] getSpecialKey() {
+ byte[] result = new byte[20];
+ Arrays.fill(result, (byte) 0);
+ System.arraycopy(Version.VERSION.getBytes(), 0, result, 0, Version.VERSION.length());
+ return result;
+ }
+
+ private String containsSpecialKey(Set<P2PDataStorage.ByteArray> collection) {
+ Optional<String> result = collection.stream().map(byteArray -> new String(byteArray.bytes).trim()).filter(s -> s.matches("^[0-9]\\.[0-9]\\.[0-9]$")).findFirst();
Is String(byte[]) safe enough? I suspect so but I'm not sure.
> // We collect the keys of the PersistableNetworkPayload items so we exclude them in our request.
// PersistedStoragePayload items don't get removed, so we don't have an issue with the case that
// an object gets removed in between PreliminaryGetDataRequest and the GetUpdatedDataRequest and we would
// miss that event if we do not load the full set or use some delta handling.
- Set<byte[]> excludedKeys = this.appendOnlyDataStoreService.getMap().keySet().stream()
- .map(e -> e.bytes)
- .collect(Collectors.toSet());
+ Set<byte[]> excludedKeys;
+ if (seedNodeRepository != null && seedNodeRepository.isSeedNode(networkNode.getNodeAddress()))
Braces around multi-line blocks really helps readability in my opinion.
> + Map<ByteArray, PersistableNetworkPayload> tmp;
+ if ("".equals(specialKey))
+ tmp = this.appendOnlyDataStoreService.getMap();
+ else {
+ tmp = this.appendOnlyDataStoreService.getMap("since " + specialKey);
+ }
Is this the separation of old and new requests for data? More comments would be helpful for understanding.
Variable name `tmp` isn't really descriptive, better naming would help.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/4233#pullrequestreview-414605107
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20200519/078e66ad/attachment-0001.html>
More information about the bisq-github
mailing list