You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tuweni.apache.org by GitBox <gi...@apache.org> on 2021/06/09 06:30:00 UTC
[GitHub] [incubator-tuweni] sudo-update opened a new pull request #274: Return first hit, simplify code
sudo-update opened a new pull request #274:
URL: https://github.com/apache/incubator-tuweni/pull/274
## PR description
"BlockchainIndex" returned 10 hits, but only the first one was observed and returned. I simplified the code to returned first hit only and removed the for loops.
## Fixed Issue(s)
fixes #267
--
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: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org
[GitHub] [incubator-tuweni] atoulme commented on a change in pull request #274: Return first hit, simplify code
Posted by GitBox <gi...@apache.org>.
atoulme commented on a change in pull request #274:
URL: https://github.com/apache/incubator-tuweni/pull/274#discussion_r648007672
##########
File path: eth-repository/src/main/kotlin/org/apache/tuweni/eth/repository/BlockchainIndex.kt
##########
@@ -483,10 +483,8 @@ class BlockchainIndex(private val indexWriter: IndexWriter) : BlockchainIndexWri
val topDocs = searcher!!.search(query, HITS)
val docs = mutableListOf<Document>()
- for (hit in topDocs.scoreDocs) {
- val doc = searcher.doc(hit.doc, setOf("_id") + fields)
- docs += doc
- }
+ val doc = searcher.doc(topDocs.scoreDocs.elementAt(0).doc, setOf("_id") + fields)
+ docs += doc
Review comment:
Looks ok from here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org
[GitHub] [incubator-tuweni] sudo-update commented on a change in pull request #274: Return first hit, simplify code
Posted by GitBox <gi...@apache.org>.
sudo-update commented on a change in pull request #274:
URL: https://github.com/apache/incubator-tuweni/pull/274#discussion_r648006868
##########
File path: eth-repository/src/main/kotlin/org/apache/tuweni/eth/repository/BlockchainIndex.kt
##########
@@ -483,10 +483,8 @@ class BlockchainIndex(private val indexWriter: IndexWriter) : BlockchainIndexWri
val topDocs = searcher!!.search(query, HITS)
val docs = mutableListOf<Document>()
- for (hit in topDocs.scoreDocs) {
- val doc = searcher.doc(hit.doc, setOf("_id") + fields)
- docs += doc
- }
+ val doc = searcher.doc(topDocs.scoreDocs.elementAt(0).doc, setOf("_id") + fields)
+ docs += doc
Review comment:
It will change the return type from List to a single element, is that okay?
--
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: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org
[GitHub] [incubator-tuweni] sudo-update commented on pull request #274: Return first hit, simplify code
Posted by GitBox <gi...@apache.org>.
sudo-update commented on pull request #274:
URL: https://github.com/apache/incubator-tuweni/pull/274#issuecomment-857451691
> Can you also take care of line 554? 10 is inlined there, should be HITS. And there's a for loop right below.
I have fixed the issue on line 554 as well.
However I cannot change the return type to a doc from a list (as requested) due to a build exception.
--
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: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org
[GitHub] [incubator-tuweni] atoulme commented on pull request #274: Return first hit, simplify code
Posted by GitBox <gi...@apache.org>.
atoulme commented on pull request #274:
URL: https://github.com/apache/incubator-tuweni/pull/274#issuecomment-857423877
Can you also take care of line 554? 10 is inlined there, should be HITS. And there's a for loop right below.
--
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: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org
[GitHub] [incubator-tuweni] sudo-update commented on pull request #274: Return first hit, simplify code
Posted by GitBox <gi...@apache.org>.
sudo-update commented on pull request #274:
URL: https://github.com/apache/incubator-tuweni/pull/274#issuecomment-858312076
I am unsure why, but GitHub was saying I was behind main, so I merged from main, just to make sure I was working from the latest codebase. I have changed EthHandlerTest.kt to not loop through 10 indices. On my end, I am building successfully.
--
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: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org
[GitHub] [incubator-tuweni] sudo-update commented on pull request #274: Return first hit, simplify code
Posted by GitBox <gi...@apache.org>.
sudo-update commented on pull request #274:
URL: https://github.com/apache/incubator-tuweni/pull/274#issuecomment-858257084
> lgtm
is there anything else I should do, in order to get this pull request 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org
[GitHub] [incubator-tuweni] atoulme merged pull request #274: Return first hit, simplify code
Posted by GitBox <gi...@apache.org>.
atoulme merged pull request #274:
URL: https://github.com/apache/incubator-tuweni/pull/274
--
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: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org
[GitHub] [incubator-tuweni] sudo-update commented on a change in pull request #274: Return first hit, simplify code
Posted by GitBox <gi...@apache.org>.
sudo-update commented on a change in pull request #274:
URL: https://github.com/apache/incubator-tuweni/pull/274#discussion_r650563269
##########
File path: eth-repository/src/main/kotlin/org/apache/tuweni/eth/repository/BlockchainIndex.kt
##########
@@ -553,16 +550,16 @@ class BlockchainIndex(private val indexWriter: IndexWriter) : BlockchainIndexWri
val topDocs = searcher!!.search(
TermQuery(Term("_type", "difficulty")),
- 10,
+ HITS,
Sort(SortField(TOTAL_DIFFICULTY.fieldName, SortField.Type.STRING, true))
)
-
- for (hit in topDocs.scoreDocs) {
- val doc = searcher.doc(hit.doc, setOf("_id"))
- val bytes = doc.getBinaryValue("_id")
+ val doc = searcher.doc(topDocs.scoreDocs.elementAt(0).doc, setOf("_id"))
Review comment:
### Looking into it now
##########
File path: eth-repository/src/main/kotlin/org/apache/tuweni/eth/repository/BlockchainIndex.kt
##########
@@ -553,16 +550,16 @@ class BlockchainIndex(private val indexWriter: IndexWriter) : BlockchainIndexWri
val topDocs = searcher!!.search(
TermQuery(Term("_type", "difficulty")),
- 10,
+ HITS,
Sort(SortField(TOTAL_DIFFICULTY.fieldName, SortField.Type.STRING, true))
)
-
- for (hit in topDocs.scoreDocs) {
- val doc = searcher.doc(hit.doc, setOf("_id"))
- val bytes = doc.getBinaryValue("_id")
+ val doc = searcher.doc(topDocs.scoreDocs.elementAt(0).doc, setOf("_id"))
Review comment:
Looking into it now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org
[GitHub] [incubator-tuweni] sudo-update commented on pull request #274: Return first hit, simplify code
Posted by GitBox <gi...@apache.org>.
sudo-update commented on pull request #274:
URL: https://github.com/apache/incubator-tuweni/pull/274#issuecomment-860271651
--
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: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org
[GitHub] [incubator-tuweni] sudo-update commented on pull request #274: Return first hit, simplify code
Posted by GitBox <gi...@apache.org>.
sudo-update commented on pull request #274:
URL: https://github.com/apache/incubator-tuweni/pull/274#issuecomment-859081938
I am unable to start the workflow check, as a first time contributor
--
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: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org
[GitHub] [incubator-tuweni] atoulme commented on pull request #274: Return first hit, simplify code
Posted by GitBox <gi...@apache.org>.
atoulme commented on pull request #274:
URL: https://github.com/apache/incubator-tuweni/pull/274#issuecomment-860162926
Running the ci now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org
[GitHub] [incubator-tuweni] atoulme commented on a change in pull request #274: Return first hit, simplify code
Posted by GitBox <gi...@apache.org>.
atoulme commented on a change in pull request #274:
URL: https://github.com/apache/incubator-tuweni/pull/274#discussion_r648005793
##########
File path: eth-repository/src/main/kotlin/org/apache/tuweni/eth/repository/BlockchainIndex.kt
##########
@@ -483,10 +483,8 @@ class BlockchainIndex(private val indexWriter: IndexWriter) : BlockchainIndexWri
val topDocs = searcher!!.search(query, HITS)
val docs = mutableListOf<Document>()
- for (hit in topDocs.scoreDocs) {
- val doc = searcher.doc(hit.doc, setOf("_id") + fields)
- docs += doc
- }
+ val doc = searcher.doc(topDocs.scoreDocs.elementAt(0).doc, setOf("_id") + fields)
+ docs += doc
Review comment:
Maybe just return the doc instead of a list here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org
[GitHub] [incubator-tuweni] atoulme commented on a change in pull request #274: Return first hit, simplify code
Posted by GitBox <gi...@apache.org>.
atoulme commented on a change in pull request #274:
URL: https://github.com/apache/incubator-tuweni/pull/274#discussion_r650479294
##########
File path: devp2p-eth/src/test/kotlin/org/apache/tuweni/devp2p/eth/EthHandlerTest.kt
##########
@@ -128,22 +128,22 @@ class EthHandlerTest {
controller = EthController(repository, MemoryTransactionPool(), requestsManager)
)
- for (i in 1..10) {
- val newBlock = createChildBlock(header)
- repository.storeBlock(newBlock)
- var txIndex = 0
- for (tx in newBlock.body.transactions) {
- repository.storeTransactionReceipt(
- TransactionReceipt(
- Bytes32.random(),
- 32L, LogsBloomFilter(), emptyList()
- ),
- txIndex, tx.hash, newBlock.header.hash
- )
- txIndex++
- }
- header = newBlock.header
- }
+
+ val newBlock = createChildBlock(header)
+ repository.storeBlock(newBlock)
+ var txIndex = 0
+
+ repository.storeTransactionReceipt(
+ TransactionReceipt(
+ Bytes32.random(),
+ 32L, LogsBloomFilter(), emptyList()
+ ),
+ txIndex, newBlock.body.transactions.hash, newBlock.header.hash
Review comment:
this is not going to compile, the list of transactions doesn't have a hash, each transaction has its own hash.
Is there a reason for modifying this test? Seems unrelated to your changes.
##########
File path: eth-repository/src/main/kotlin/org/apache/tuweni/eth/repository/BlockchainIndex.kt
##########
@@ -553,16 +550,16 @@ class BlockchainIndex(private val indexWriter: IndexWriter) : BlockchainIndexWri
val topDocs = searcher!!.search(
TermQuery(Term("_type", "difficulty")),
- 10,
+ HITS,
Sort(SortField(TOTAL_DIFFICULTY.fieldName, SortField.Type.STRING, true))
)
-
- for (hit in topDocs.scoreDocs) {
- val doc = searcher.doc(hit.doc, setOf("_id"))
- val bytes = doc.getBinaryValue("_id")
+ val doc = searcher.doc(topDocs.scoreDocs.elementAt(0).doc, setOf("_id"))
Review comment:
What if scoreDocs is empty? Would be good to check the element exists, and return null early otherwise.
--
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: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org
[GitHub] [incubator-tuweni] sudo-update commented on a change in pull request #274: Return first hit, simplify code
Posted by GitBox <gi...@apache.org>.
sudo-update commented on a change in pull request #274:
URL: https://github.com/apache/incubator-tuweni/pull/274#discussion_r648006868
##########
File path: eth-repository/src/main/kotlin/org/apache/tuweni/eth/repository/BlockchainIndex.kt
##########
@@ -483,10 +483,8 @@ class BlockchainIndex(private val indexWriter: IndexWriter) : BlockchainIndexWri
val topDocs = searcher!!.search(query, HITS)
val docs = mutableListOf<Document>()
- for (hit in topDocs.scoreDocs) {
- val doc = searcher.doc(hit.doc, setOf("_id") + fields)
- docs += doc
- }
+ val doc = searcher.doc(topDocs.scoreDocs.elementAt(0).doc, setOf("_id") + fields)
+ docs += doc
Review comment:
It will change the return type from List<Document> to a single Document, is that okay?
--
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: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org
[GitHub] [incubator-tuweni] atoulme commented on pull request #274: Return first hit, simplify code
Posted by GitBox <gi...@apache.org>.
atoulme commented on pull request #274:
URL: https://github.com/apache/incubator-tuweni/pull/274#issuecomment-858257690
You have to get a successful build. Not sure why it fails right now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org
[GitHub] [incubator-tuweni] sudo-update commented on a change in pull request #274: Return first hit, simplify code
Posted by GitBox <gi...@apache.org>.
sudo-update commented on a change in pull request #274:
URL: https://github.com/apache/incubator-tuweni/pull/274#discussion_r648017710
##########
File path: eth-repository/src/main/kotlin/org/apache/tuweni/eth/repository/BlockchainIndex.kt
##########
@@ -483,10 +483,8 @@ class BlockchainIndex(private val indexWriter: IndexWriter) : BlockchainIndexWri
val topDocs = searcher!!.search(query, HITS)
val docs = mutableListOf<Document>()
- for (hit in topDocs.scoreDocs) {
- val doc = searcher.doc(hit.doc, setOf("_id") + fields)
- docs += doc
- }
+ val doc = searcher.doc(topDocs.scoreDocs.elementAt(0).doc, setOf("_id") + fields)
+ docs += doc
Review comment:
When I make the above change, I get this error:
Task :eth-repository:compileKotlin FAILED
e: /home/gradle/eth-repository/src/main/kotlin/org/apache/tuweni/eth/repository/BlockchainIndex.kt: (485, 14): Type mismatch: inferred type is Document! but List<Document> was expected
FAILURE: Build failed with an exception.
--
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: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org